Re: [PATCH] btrfs-progs: use zero-range ioctl to verify discard support

["Martin K. Petersen" <martin.petersen@oracle.com>]

Hi Anand!

> However, this isn't always reliable. For example, on a virtual device,
> discard_granularity may be non-zero (e.g., 512), but an actual
> ioctl(BLKDISCARD) call still fails with EOPNOTSUPP.

Regrettably this has changed as a result of block layer code shuffling.

Documentation/ABI/stable/sysfs-block states:

  What: /sys/block/<disk>/queue/discard_granularity
  [...]
  A discard_granularity of 0 means that the device does not support
  discard functionality.

The semantics visible to userspace were inadvertently changed and I
would actually like to see that addressed. Reporting a default
granularity of 512 when discard is unsupported has broken other apps
too.

> One workaround is to also check discard_max_bytes for a non-zero value.
>
> $ cat /sys/block/sda/queue/discard_max_bytes
> 0
>
> But a better and more direct way is to test discard support by issuing
> a BLKDISCARD ioctl with a zero-length range. If this call fails with
> EOPNOTSUPP, discard isn't supported.

In SCSI, a WRITE SAME operation with a transfer length of 0 translates
to "entire device". And in NVMe, a WRITE ZEROES block count of 0
translates to "1 logical block".

While we currently catch zero-length discards in the block layer before
they get passed down the stack, I am concerned about userland
applications relying on these operations being non-destructive in the
zero-length case. Something which can't be expressed at the storage
protocol level.

So I would prefer for you to validate that both discard_granularity and
discover_max_bytes are non-zero. Obviously we have no plans to remove
the existing check in blk_ioctl_discard(). But if we mess something up
by accident, data loss could result. A zero transfer length write
operation is not inherently safe. Whereas inspecting the queue topology
is entirely innocuous.

-- 
Martin K. Petersen