Re: [PATCH 23/23] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Thu, Mar 30, 2017 at 11:29:29AM -0400, Martin K. Petersen wrote:
> "h...@lst.de"  writes:
> 
> > Jens, any opinion?  I'd like to remove it too, but I fear it might
> > break things.  We could deprecate it first with a warning when read
> > and then remove it a few releases down the road.
> 
> I know of several apps that check this variable (as opposed to the
> ioctl).

The above was in reference to both methods..

Re: [PATCH 23/23] block: remove the discard_zeroes_data flag

[Martin K. Petersen]

"h...@lst.de"  writes:

> Jens, any opinion?  I'd like to remove it too, but I fear it might
> break things.  We could deprecate it first with a warning when read
> and then remove it a few releases down the road.

I know of several apps that check this variable (as opposed to the
ioctl).

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 23/23] block: remove the discard_zeroes_data flag

[h...@lst.de]

On Tue, Mar 28, 2017 at 05:00:48PM +, Bart Van Assche wrote:
> 
> It seems to me like the documentation in Documentation/ABI/testing/sysfs-block
> and the above code are not in sync. I think the above code will cause reading
> from the discard_zeroes_data attribute to return an empty string ("") instead
> of "0\n".

Thanks, fine with me.

> 
> BTW, my personal preference is to remove this attribute entirely because 
> keeping
> it will cause confusion, no matter how well we document the behavior of this
> attribute.

Jens, any opinion?  I'd like to remove it too, but I fear it might
break things.  We could deprecate it first with a warning when read
and then remove it a few releases down the road.

Re: [PATCH 23/23] block: remove the discard_zeroes_data flag

[Paolo Bonzini]

On 28/03/2017 19:00, Bart Van Assche wrote:
> On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
>> Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
>> kill this hack.
>>
>> [ ... ]
>>
>> diff --git a/Documentation/ABI/testing/sysfs-block 
>> b/Documentation/ABI/testing/sysfs-block
>> index 2da04ce6aeef..dea212db9df3 100644
>> --- a/Documentation/ABI/testing/sysfs-block
>> +++ b/Documentation/ABI/testing/sysfs-block
>> @@ -213,14 +213,8 @@ What:   
>> /sys/block//queue/discard_zeroes_data
>>  Date:   May 2011
>>  Contact:Martin K. Petersen 
>>  Description:
>> -Devices that support discard functionality may return
>> -stale or random data when a previously discarded block
>> -is read back. This can cause problems if the filesystem
>> -expects discarded blocks to be explicitly cleared. If a
>> -device reports that it deterministically returns zeroes
>> -when a discarded area is read the discard_zeroes_data
>> -parameter will be set to one. Otherwise it will be 0 and
>> -the result of reading a discarded area is undefined.
>> +Will always return 0.  Don't rely on any specific behavior
>> +for discards, and don't read this file.
>>  
>>  What:   /sys/block//queue/write_same_max_bytes
>>  Date:   January 2012
>>
>> [ ... ]
>>
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -208,7 +208,7 @@ static ssize_t queue_discard_max_store(struct 
>> request_queue *q,
>>  
>>  static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char 
>> *page)
>>  {
>> -return queue_var_show(queue_discard_zeroes_data(q), page);
>> +return 0;
>>  }
> 
> Hello Christoph,
> 
> It seems to me like the documentation in Documentation/ABI/testing/sysfs-block
> and the above code are not in sync. I think the above code will cause reading
> from the discard_zeroes_data attribute to return an empty string ("") instead
> of "0\n".
> 
> BTW, my personal preference is to remove this attribute entirely because 
> keeping
> it will cause confusion, no matter how well we document the behavior of this
> attribute.

If you remove it, you should probably remove the BLKDISCARDZEROES ioctl too.

That said, the issue with discard_zeroes_data is that it is badly
defined; it was defined as "if I unmap X, will it read as zeroes?" but
this is not how the SCSI standard defines e.g. the UNMAP command with
LBPRZ=1.  But knowing something like LBPRZ ("if something is unmapped,
will it read as zeroes?") _would_ actually be useful for userspace.
This will be especially true once sd maps lseek(SEEK_HOLE/SEEK_DATA) to
the SCSI GET LBA STATUS command, or once dm-thin supports them.

Secondarily, if the former returns 1, userspace is also interested in
knowing "can REQ_OP_WRITE_ZEROES+REQ_UNMAP ever unmap anything?", i.e.
whether BLKDEV_ZERO_NOFALLBACK will ever return anything but
-EOPNOTSUPP.  For SCSI, this should intuitively mean whether LBPWS or
LBPWS10 are set, but the details depend on how the sd driver implements
REQ_OP_WRITE_ZEROES with REQ_UNMAP.

Paolo

Re: [PATCH 23/23] block: remove the discard_zeroes_data flag

[Bart Van Assche]

On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
> kill this hack.
> 
> [ ... ]
>
> diff --git a/Documentation/ABI/testing/sysfs-block 
> b/Documentation/ABI/testing/sysfs-block
> index 2da04ce6aeef..dea212db9df3 100644
> --- a/Documentation/ABI/testing/sysfs-block
> +++ b/Documentation/ABI/testing/sysfs-block
> @@ -213,14 +213,8 @@ What:
> /sys/block//queue/discard_zeroes_data
>  Date:May 2011
>  Contact: Martin K. Petersen 
>  Description:
> - Devices that support discard functionality may return
> - stale or random data when a previously discarded block
> - is read back. This can cause problems if the filesystem
> - expects discarded blocks to be explicitly cleared. If a
> - device reports that it deterministically returns zeroes
> - when a discarded area is read the discard_zeroes_data
> - parameter will be set to one. Otherwise it will be 0 and
> - the result of reading a discarded area is undefined.
> + Will always return 0.  Don't rely on any specific behavior
> + for discards, and don't read this file.
>  
>  What:/sys/block//queue/write_same_max_bytes
>  Date:January 2012
>
> [ ... ]
>
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -208,7 +208,7 @@ static ssize_t queue_discard_max_store(struct 
> request_queue *q,
>  
>  static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char 
> *page)
>  {
> - return queue_var_show(queue_discard_zeroes_data(q), page);
> + return 0;
>  }

Hello Christoph,

It seems to me like the documentation in Documentation/ABI/testing/sysfs-block
and the above code are not in sync. I think the above code will cause reading
from the discard_zeroes_data attribute to return an empty string ("") instead
of "0\n".

BTW, my personal preference is to remove this attribute entirely because keeping
it will cause confusion, no matter how well we document the behavior of this
attribute.

Thanks,

Bart.

[PATCH 23/23] block: remove the discard_zeroes_data flag

[Christoph Hellwig]

Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we can
kill this hack.

Signed-off-by: Christoph Hellwig 
---
 Documentation/ABI/testing/sysfs-block | 10 ++
 Documentation/block/queue-sysfs.txt   |  5 -
 block/blk-lib.c   |  7 +--
 block/blk-settings.c  |  3 ---
 block/blk-sysfs.c |  2 +-
 block/compat_ioctl.c  |  2 +-
 block/ioctl.c |  2 +-
 drivers/ata/libata-scsi.c |  2 +-
 drivers/block/drbd/drbd_main.c|  2 --
 drivers/block/drbd/drbd_nl.c  |  7 +--
 drivers/block/loop.c  |  2 --
 drivers/block/mtip32xx/mtip32xx.c |  1 -
 drivers/block/nbd.c   |  1 -
 drivers/md/dm-cache-target.c  |  1 -
 drivers/md/dm-crypt.c |  1 -
 drivers/md/dm-raid.c  |  6 +++---
 drivers/md/dm-raid1.c |  1 -
 drivers/md/dm-table.c | 19 ---
 drivers/md/dm-thin.c  |  2 --
 drivers/md/raid5.c| 12 +---
 drivers/scsi/sd.c |  7 ---
 drivers/target/target_core_device.c   |  2 +-
 include/linux/blkdev.h| 15 ---
 include/linux/device-mapper.h |  5 -
 24 files changed, 13 insertions(+), 104 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-block 
b/Documentation/ABI/testing/sysfs-block
index 2da04ce6aeef..dea212db9df3 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -213,14 +213,8 @@ What:  
/sys/block//queue/discard_zeroes_data
 Date:  May 2011
 Contact:   Martin K. Petersen 
 Description:
-   Devices that support discard functionality may return
-   stale or random data when a previously discarded block
-   is read back. This can cause problems if the filesystem
-   expects discarded blocks to be explicitly cleared. If a
-   device reports that it deterministically returns zeroes
-   when a discarded area is read the discard_zeroes_data
-   parameter will be set to one. Otherwise it will be 0 and
-   the result of reading a discarded area is undefined.
+   Will always return 0.  Don't rely on any specific behavior
+   for discards, and don't read this file.
 
 What:  /sys/block//queue/write_same_max_bytes
 Date:  January 2012
diff --git a/Documentation/block/queue-sysfs.txt 
b/Documentation/block/queue-sysfs.txt
index c0a3bb5a6e4e..009150ed7db8 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -43,11 +43,6 @@ large discards are issued, setting this value lower will 
make Linux issue
 smaller discards and potentially help reduce latencies induced by large
 discard operations.
 
-discard_zeroes_data (RO)
-
-When read, this file will show if the discarded block are zeroed by the
-device or not. If its value is '1' the blocks are zeroed otherwise not.
-
 hw_sector_size (RO)
 ---
 This is the hardware sector size of the device, in bytes.
diff --git a/block/blk-lib.c b/block/blk-lib.c
index 153ca59393a7..be44d2725ede 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -37,17 +37,12 @@ int __blkdev_issue_discard(struct block_device *bdev, 
sector_t sector,
return -ENXIO;
 
if (flags & BLKDEV_DISCARD_SECURE) {
-   if (flags & BLKDEV_DISCARD_ZERO)
-   return -EOPNOTSUPP;
if (!blk_queue_secure_erase(q))
return -EOPNOTSUPP;
op = REQ_OP_SECURE_ERASE;
} else {
if (!blk_queue_discard(q))
return -EOPNOTSUPP;
-   if ((flags & BLKDEV_DISCARD_ZERO) &&
-   !q->limits.discard_zeroes_data)
-   return -EOPNOTSUPP;
op = REQ_OP_DISCARD;
}
 
@@ -126,7 +121,7 @@ int blkdev_issue_discard(struct block_device *bdev, 
sector_t sector,
);
if (!ret && bio) {
ret = submit_bio_wait(bio);
-   if (ret == -EOPNOTSUPP && !(flags & BLKDEV_DISCARD_ZERO))
+   if (ret == -EOPNOTSUPP)
ret = 0;
bio_put(bio);
}
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 9d515ae3a405..fe2794986eb9 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -104,7 +104,6 @@ void blk_set_default_limits(struct queue_limits *lim)
lim->discard_granularity = 0;
lim->discard_alignment = 0;
lim->discard_misaligned = 0;
-   lim->discard_zeroes_data = 0;
lim->logical_block_size = lim->physical_block_size = lim->io_min = 512;
lim->bounce_pfn = (unsigned