[PATCH] sys/block: Add discard_support entry

[PATCH] sys/block: Add discard_support entry

[Lukas Czerner]

Currently there is no convenient way how to get the information from
userspace whether or not the block device supports discard other
than try to call BLKDISCARD ioctl and check the result, which is not
really good way to do it.

We already have BLKDISCARDZEROES ioctl and
/sys/block/sda/queue/discard_zeroes_data to tell whether  subsequent
reads after discard returns all zeroes. This commit adds at least sysfs
interface /sys/block/sda/queue/discard_support to tell whether device
support discard.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: Jeff Moyer <jmoyer@redhat.com>
---
 block/blk-sysfs.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index bd23631..96f1cca 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -160,6 +160,11 @@ static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char *pag
 	return queue_var_show(queue_discard_zeroes_data(q), page);
 }
 
+static ssize_t queue_discard_support_show(struct request_queue *q, char *page)
+{
+	return queue_var_show(blk_queue_discard(q), page);
+}
+
 static ssize_t
 queue_max_sectors_store(struct request_queue *q, const char *page, size_t count)
 {
@@ -349,6 +354,11 @@ static struct queue_sysfs_entry queue_discard_zeroes_data_entry = {
 	.show = queue_discard_zeroes_data_show,
 };
 
+static struct queue_sysfs_entry queue_discard_support_entry = {
+	.attr = {.name = "discard_support", .mode = S_IRUGO },
+	.show = queue_discard_support_show,
+};
+
 static struct queue_sysfs_entry queue_nonrot_entry = {
 	.attr = {.name = "rotational", .mode = S_IRUGO | S_IWUSR },
 	.show = queue_show_nonrot,
@@ -396,6 +406,7 @@ static struct attribute *default_attrs[] = {
 	&queue_discard_granularity_entry.attr,
 	&queue_discard_max_entry.attr,
 	&queue_discard_zeroes_data_entry.attr,
+	&queue_discard_support_entry.attr,
 	&queue_nonrot_entry.attr,
 	&queue_nomerges_entry.attr,
 	&queue_rq_affinity_entry.attr,
-- 
1.7.4.4

Re: [PATCH] sys/block: Add discard_support entry

[Martin K. Petersen]

>>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:

Lukas> Currently there is no convenient way how to get the information
Lukas> from userspace whether or not the block device supports discard
Lukas> other than try to call BLKDISCARD ioctl and check the result,
Lukas> which is not really good way to do it.

Not true. If discard_max_bytes is > 0 then discard is supported.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sys/block: Add discard_support entry

[Lukas Czerner]

On Mon, 2 May 2011, Martin K. Petersen wrote:

> >>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:
> 
> Lukas> Currently there is no convenient way how to get the information
> Lukas> from userspace whether or not the block device supports discard
> Lukas> other than try to call BLKDISCARD ioctl and check the result,
> Lukas> which is not really good way to do it.
> 
> Not true. If discard_max_bytes is > 0 then discard is supported.
> 

Yes I thought about that, but it is not actually *very* obvious and I
was not even sure it works this way all the time. I think it is better
to have a real sysfs file.

Thanks!
-Lukas

Re: [PATCH] sys/block: Add discard_support entry

[Martin K. Petersen]

>>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:

>> Not true. If discard_max_bytes is > 0 then discard is supported.
>> 

Lukas> Yes I thought about that, but it is not actually *very* obvious
Lukas> and I was not even sure it works this way all the time. I think
Lukas> it is better to have a real sysfs file.

To me it's pretty obvious that if the device reports a max discard size
of 0 then discard is not supported.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sys/block: Add discard_support entry

[Lukas Czerner]

On Mon, 2 May 2011, Martin K. Petersen wrote:

> >>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:
> 
> >> Not true. If discard_max_bytes is > 0 then discard is supported.
> >> 
> 
> Lukas> Yes I thought about that, but it is not actually *very* obvious
> Lukas> and I was not even sure it works this way all the time. I think
> Lukas> it is better to have a real sysfs file.
> 
> To me it's pretty obvious that if the device reports a max discard size
> of 0 then discard is not supported.
> 

So is that documented somewhere ? Because to me it is really not obvious
whether 0 means that the device does not support discard, or the device
just does not report discard_max_bytes at all.

-Lukas

Re: [PATCH] sys/block: Add discard_support entry

[Martin K. Petersen]

>>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:

Lukas,

Lukas> So is that documented somewhere ? Because to me it is really not
Lukas> obvious whether 0 means that the device does not support discard,
Lukas> or the device just does not report discard_max_bytes at all.

discard_max_bytes does not necessarily correspond to a single value
reported by the device. Not all device types have the notion of such a
setting.

discard_max_bytes is calculated at the bottom of the stack depending on
a combination of heuristics and metrics reported by the device. The same
goes for granularity and alignment.

So I guess the important thing here is that these values may or may not
correspond directly to the underlying device (hopefully they will
:). But the topology information *itself* is the interface. That's the
whole point. To detach the top of the stack from the intricacies of the
hardware.

You should never have to ask yourself whether a device supports
reporting discard_max_bytes. If a device supports discard then
discard_max_bytes will be set. Always. Same goes for granularity and
alignment. Regardless of whether the device reports these values or
not. We'll fill out the blanks.

This is true for the rest of the topology information as well. If the
device does not report a physical block size (most don't) we'll set it
to match the logical block size. Etc.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] sys/block: Add discard_support entry

[Greg KH]

On Mon, May 02, 2011 at 01:25:04PM +0200, Lukas Czerner wrote:
> Currently there is no convenient way how to get the information from
> userspace whether or not the block device supports discard other
> than try to call BLKDISCARD ioctl and check the result, which is not
> really good way to do it.
> 
> We already have BLKDISCARDZEROES ioctl and
> /sys/block/sda/queue/discard_zeroes_data to tell whether  subsequent
> reads after discard returns all zeroes. This commit adds at least sysfs
> interface /sys/block/sda/queue/discard_support to tell whether device
> support discard.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Cc: Jens Axboe <jaxboe@fusionio.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> ---
>  block/blk-sysfs.c |   11 +++++++++++

If you ever change/add/remove a sysfs file, you also need a
Documentation/ABI/ entry.  Please include that in the next revision of
this patch, if you do one.

thanks,

greg k-h

Re: [PATCH] sys/block: Add discard_support entry

[Lukas Czerner]

On Mon, 2 May 2011, Martin K. Petersen wrote:

> >>>>> "Lukas" == Lukas Czerner <lczerner@redhat.com> writes:
> 
> Lukas,
> 
> Lukas> So is that documented somewhere ? Because to me it is really not
> Lukas> obvious whether 0 means that the device does not support discard,
> Lukas> or the device just does not report discard_max_bytes at all.
> 
> discard_max_bytes does not necessarily correspond to a single value
> reported by the device. Not all device types have the notion of such a
> setting.
> 
> discard_max_bytes is calculated at the bottom of the stack depending on
> a combination of heuristics and metrics reported by the device. The same
> goes for granularity and alignment.
> 
> So I guess the important thing here is that these values may or may not
> correspond directly to the underlying device (hopefully they will
> :). But the topology information *itself* is the interface. That's the
> whole point. To detach the top of the stack from the intricacies of the
> hardware.
> 
> You should never have to ask yourself whether a device supports
> reporting discard_max_bytes. If a device supports discard then
> discard_max_bytes will be set. Always. Same goes for granularity and
> alignment. Regardless of whether the device reports these values or
> not. We'll fill out the blanks.
> 
> This is true for the rest of the topology information as well. If the
> device does not report a physical block size (most don't) we'll set it
> to match the logical block size. Etc.
> 

Hi Martin,

Thanks for explanation, now I know that I can rely on discard_max_bytes
to be set correctly and that this has "discard supported" information in
it intentionally. Would you mind documenting the whole sysfs discard_*
stuff in Documentation/ABI as you seem to very well understand how those
information are being created ?

I agree that the discard_support entry is not needed.

Thanks!
-Lukas

Re: [PATCH] sys/block: Add discard_support entry

[Lukas Czerner]

On Mon, 2 May 2011, Greg KH wrote:

> On Mon, May 02, 2011 at 01:25:04PM +0200, Lukas Czerner wrote:
> > Currently there is no convenient way how to get the information from
> > userspace whether or not the block device supports discard other
> > than try to call BLKDISCARD ioctl and check the result, which is not
> > really good way to do it.
> > 
> > We already have BLKDISCARDZEROES ioctl and
> > /sys/block/sda/queue/discard_zeroes_data to tell whether  subsequent
> > reads after discard returns all zeroes. This commit adds at least sysfs
> > interface /sys/block/sda/queue/discard_support to tell whether device
> > support discard.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Cc: Jens Axboe <jaxboe@fusionio.com>
> > Cc: Jeff Moyer <jmoyer@redhat.com>
> > ---
> >  block/blk-sysfs.c |   11 +++++++++++
> 
> If you ever change/add/remove a sysfs file, you also need a
> Documentation/ABI/ entry.  Please include that in the next revision of
> this patch, if you do one.
> 
> thanks,

Oh, thanks Greg I will do it next time as I will not do a next revision
of this patch.

-Lukas

> 
> greg k-h
>