Re: [PATCH v2 1/2] block: Read max write zeroes once for __blkdev_issue_write_zeroes()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 17/08/24 04:33PM, John Garry wrote:
On 16/08/2024 19:34, Nitesh Shetty wrote:
On 15/08/24 04:32PM, John Garry wrote:
As reported in [0], we may get a hang when formatting a XFS FS on a RAID0
drive.

Commit 73a768d5f955 ("block: factor out a blk_write_zeroes_limit helper")
changed __blkdev_issue_write_zeroes() to read the max write zeroes
value in the loop. This is not safe as max write zeroes may change in
value. Specifically for the case of [0], the value goes to 0, and we get
an infinite loop.

Lift the limit reading out of the loop.

[0] https://urldefense.com/v3/__https://lore.kernel.org/linux- xfs/4d31268f-310b-4220-88a2-e191c3932a82@xxxxxxxxxx/T/*t__;Iw!! ACWV5N9M2RV99hQ! KNrgu0c216k_Y_2RLxTasjxizyhbN8eKD61JwIwDxT5OJJDamER6hw1nvf5biNMqQLaLl9PqC2qRUDdHlrGF7g$
Fixes: 73a768d5f955 ("block: factor out a blk_write_zeroes_limit helper")
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
---
block/blk-lib.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9f735efa6c94..83eb7761c2bf 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -111,13 +111,20 @@ static sector_t bio_write_zeroes_limit(struct block_device *bdev)
        (UINT_MAX >> SECTOR_SHIFT) & ~bs_mask);
}

+/*
+ * There is no reliable way for the SCSI subsystem to determine whether a + * device supports a WRITE SAME operation without actually performing a write
+ * to media. As a result, write_zeroes is enabled by default and will be
+ * disabled if a zeroing operation subsequently fails. This means that this
+ * queue limit is likely to change at runtime.
+ */
static void __blkdev_issue_write_zeroes(struct block_device *bdev,
        sector_t sector, sector_t nr_sects, gfp_t gfp_mask,
-        struct bio **biop, unsigned flags)
+        struct bio **biop, unsigned flags, sector_t limit)
{
+
    while (nr_sects) {
-        unsigned int len = min_t(sector_t, nr_sects,
-                bio_write_zeroes_limit(bdev));
+        unsigned int len = min(nr_sects, limit);

I feel changes something like below will simplify the whole patch.

--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -115,9 +115,13 @@ static void __blkdev_issue_write_zeroes(struct block_device *bdev,
         sector_t sector, sector_t nr_sects, gfp_t gfp_mask,
         struct bio **biop, unsigned flags)
 {
+    sector_t limit = bio_write_zeroes_limit(bdev);
+
+    if (!limit)
+        return;

Just this?

If yes, then __blkdev_issue_write_zeroes() does not return an error code, so can we tell whether the zeroes were actually issued?

I failed to see this.

Furthermore, I would rather limit points at which we call bio_write_zeroes_limit()

BTW, I am going on a short vacation now, so I can't quickly rework this (if that was actually required).


Reviewed-by: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>





[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux