Re: [PATCH] loop: Fix missing discard support when using LOOP_CONFIGURE

[Kristian Klausen]

On 18.06.2021 01.03, Ming Lei wrote:
> On Fri, Jun 18, 2021 at 12:11:57AM +0200, Kristian Klausen wrote:
>
> Commit log?

I will try to come up with a commit message. AFAIU the missing call to 
loop_config_discard() causes the block device to report "no discard 
support" (best-case) or stale/incorrect discard support (ex: if the loop 
device was configured with LOOP_SET_STATUS(64) and later LOOP_CONFIGURE).

> > Cc: <stable@xxxxxxxxxxxxxxx> # 5.8.x-
> > Fixes: 3448914e8cc5 ("loop: Add LOOP_CONFIGURE ioctl")
> > Signed-off-by: Kristian Klausen <kristian@xxxxxxxxxx>
> > ---
> > Tested like so (without the patch):
> > losetup 2.37<= uses LOOP_CONFIGURE instead of LOOP_SET_STATUS64[1]
> >
> > # fallocate -l100M disk.img
> > # rmmod loop
> > # losetup --version
> > losetup from util-linux 2.36.2
> > # losetup --find --show disk.img
> > /dev/loop0
> > # grep '' /sys/devices/virtual/block/loop0/queue/*discard*
> > /sys/devices/virtual/block/loop0/queue/discard_granularity:4096
> > /sys/devices/virtual/block/loop0/queue/discard_max_bytes:4294966784
> > /sys/devices/virtual/block/loop0/queue/discard_max_hw_bytes:4294966784
> > /sys/devices/virtual/block/loop0/queue/discard_zeroes_data:0
> > /sys/devices/virtual/block/loop0/queue/max_discard_segments:1
> > # losetup -d /dev/loop0
> > # [update util-linux]
> > # losetup --version
> > losetup from util-linux 2.37
> > # rmmod loop
> > # losetup --find --show disk.img
> > /dev/loop0
> > # grep '' /sys/devices/virtual/block/loop0/queue/*discard*
> > /sys/devices/virtual/block/loop0/queue/discard_granularity:0
> > /sys/devices/virtual/block/loop0/queue/discard_max_bytes:0
> > /sys/devices/virtual/block/loop0/queue/discard_max_hw_bytes:0
> > /sys/devices/virtual/block/loop0/queue/discard_zeroes_data:0
> > /sys/devices/virtual/block/loop0/queue/max_discard_segments:1
> >
> >
> > With the patch applied:
> >
> > # losetup --version
> > losetup from util-linux 2.37
> > # rmmod loop
> > # losetup --find --show disk.img
> > /dev/loop0
> > # grep '' /sys/devices/virtual/block/loop0/queue/*discard*
> > /sys/devices/virtual/block/loop0/queue/discard_granularity:4096
> > /sys/devices/virtual/block/loop0/queue/discard_max_bytes:4294966784
> > /sys/devices/virtual/block/loop0/queue/discard_max_hw_bytes:4294966784
> > /sys/devices/virtual/block/loop0/queue/discard_zeroes_data:0
> > /sys/devices/virtual/block/loop0/queue/max_discard_segments:1
> >
> > [1] https://github.com/karelzak/util-linux/pull/1152
> >
> >   drivers/block/loop.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 76e12f3482a9..ec957f6d8a49 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1168,6 +1168,8 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
> >   	if (partscan)
> >   		lo->lo_disk->flags &= ~GENHD_FL_NO_PART_SCAN;
> >   
> > +	loop_config_discard(lo);
> > +
> It could be better to move loop_config_discard() around
> loop_update_rotational/loop_update_dio(), then we setup everything
> before updating loop as Lo_bound.

I will move it before loop_update_rotational().

> Otherwise, this patch looks fine.
>
>
> Thanks,
> Ming