On 2020/8/4 16:14, Ming Lei wrote:

On Tue, Aug 04, 2020 at 03:50:32PM +0800, Coly Li wrote:

quoted

On 2020/8/4 14:01, Ming Lei wrote:

quoted

On Tue, Aug 04, 2020 at 12:29:07PM +0800, Coly Li wrote:

quoted

On 2020/8/4 12:15, Ming Lei wrote:

quoted

In case of block device backend, if the backend supports discard, the
loop device will set queue flag of QUEUE_FLAG_DISCARD.

However, limits.discard_granularity isn't setup, and this way is wrong,
see the following description in Documentation/ABI/testing/sysfs-block:

	A discard_granularity of 0 means that the device does not support
	discard functionality.

Especially 9b15d109a6b2 ("block: improve discard bio alignment in
__blkdev_issue_discard()") starts to take q->limits.discard_granularity
for computing max discard sectors. And zero discard granularity causes
kernel oops[1].

Fix the issue by set up discard granularity and alignment.

[snipped]

quoted

quoted

quoted

Hi Ming,

I did similar change, it can avoid the panic or 0 length discard bio.
But yesterday I realize the discard request to loop device should not go
into __blkdev_issue_discard(). As commit c52abf563049 ("loop: Better
discard support for block devices") mentioned it should go into
blkdev_issue_zeroout(), this is why in loop_config_discard() the
max_discard_sectors is set to backingq->limits.max_write_zeroes_sectors.

That commit meant REQ_OP_DISCARD on a loop device is translated into
blkdev_issue_zeroout(), because REQ_OP_DISCARD is handled as
file->f_op->fallocate(FALLOC_FL_PUNCH_HOLE), which will cause
"subsequent reads from this range will return zeroes".

quoted

Now I am looking at the problem why discard request on loop device
doesn't go into blkdev_issue_zeroout().

No, that is correct behavior, since loop can support discard or zeroout.

If QUEUE_FLAG_DISCARD is set, either discard_granularity or max discard
sectors shouldn't be zero.

This patch shouldn't set discard_granularity if limits.max_write_zeroes_sectors
is zero, will fix it in V2.

quoted

With the above change, the discard is very slow on loop device with
backing device. In my testing, mkfs.xfs on /dev/loop0 does not complete
in 20 minutes, each discard request is only 4097 sectors.

I'd suggest you to check the discard related queue limits, and see why
each discard request just sends 4097 sectors.

Or we need to mirror underlying queue's discard_granularity too? Can you
try the following patch?

Can I know the exact command line to reproduce the panic ?

modprobe scsi_debug delay=0 dev_size_mb=2048 max_queue=1

losetup -f /dev/sdc --direct-io=on   #suppose /dev/sdc is the scsi_debug LUN

mkfs.ext4 /dev/loop0				#suppose loop0 is the loop backed by /dev/sdc

mount /dev/loop0 /mnt

cd /mnt
dbench -t 20 -s 64

Thanks for the information. With the above command lines, I also find a
special case that I can see a 0 byte request triggers a similar BUG()
panic --- when the discard LBA is 0, and loop device driver
queue->limits.discard_granularity is 0.

quoted

I try to use blkdiscard, or dbench -D /mount_point_to_loop0, and see all
the discard requests go into blkdev_fallocate()=>blkdev_issue_zeroout().
No request goes into __blkdev_issue_discard().

The issue isn't related with backend queue, and it is triggered on
loop's request.

Yes it is clear to me now. Beside the loop device driver,
__blkdev_issue_discard() should also be fixed to tolerate the buggy
queue->limits.discard_granularity, in case some other driver does
similar mistake, or a unlucky downstream kernel maintainer missing your
loop device driver fix.

Yes, please post the v2 patch, now I can help to review this patch.

Thanks for your hint :-)

Coly Li