Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[yebin (H)]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]



On 2023/11/7 19:12, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> If one of the underlying disks of raid or dm is set to read-only, then
> each io will generate new log, which will cause message storm. This
> environment is indeed problematic, however we can't make sure our
> naive custormer won't do this, hence use pr_warn_ratelimited() to
> prevent message storm in this case.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   block/blk-core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9d51e9894ece..fdf25b8d6e78 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -501,8 +501,8 @@ static inline void bio_check_ro(struct bio *bio)
>   	if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
>   		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
>   			return;
> -		pr_warn("Trying to write to read-only block-device %pg\n",
> -			bio->bi_bdev);
> +		pr_warn_ratelimited("Trying to write to read-only block-device %pg\n",
> +				    bio->bi_bdev);
Acctually, before commit  57e95e4670d1 ("block: fix and cleanup 
bio_check_ro") , there's only print warning once.
I wrote a patch earlier, set GD_ROWR_WARNED flag for disk after emit 
warning. You can look at the patch in the
attachment, Is it possible to solve your problem.
>   		/* Older lvm-tools actually trigger this */
>   	}
>   }


[-- Attachment #2: 0001-block-only-print-warning-once-when-write-to-readonly.patch --]
[-- Type: text/x-patch, Size: 2501 bytes --]

From 82b7dd41eb447e9fdd3d7c5d5e3a002a9f284d82 Mon Sep 17 00:00:00 2001
From: Ye Bin <yebin10@huawei.com>
Date: Wed, 9 Aug 2023 16:11:39 +0800
Subject: [PATCH] block: only print warning once when write to readonly bdev

After commit 57e95e4670d1 there will print warning unconditionally.As a result,
alarm logs are flooded.
To solve above issue, introduce GD_ROWR_WARNED state for every partition to
record if warning has been printed.

Fixes: 57e95e4670d1 ("block: fix and cleanup bio_check_ro")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 block/blk-core.c       | 3 +++
 block/genhd.c          | 1 +
 block/ioctl.c          | 6 +++++-
 include/linux/blkdev.h | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9d51e9894ece..69d9757a013a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -501,6 +501,9 @@ static inline void bio_check_ro(struct bio *bio)
 	if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
 		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
 			return;
+		if (test_and_set_bit(GD_ROWR_WARNED,
+				     &bio->bi_bdev->bd_disk->state))
+			return;
 		pr_warn("Trying to write to read-only block-device %pg\n",
 			bio->bi_bdev);
 		/* Older lvm-tools actually trigger this */
diff --git a/block/genhd.c b/block/genhd.c
index c9d06f72c587..c05d2cd4a87b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1455,6 +1455,7 @@ void set_disk_ro(struct gendisk *disk, bool read_only)
 		if (!test_and_clear_bit(GD_READ_ONLY, &disk->state))
 			return;
 	}
+	clear_bit(GD_ROWR_WARNED, &disk->state);
 	set_disk_ro_uevent(disk, read_only);
 }
 EXPORT_SYMBOL(set_disk_ro);
diff --git a/block/ioctl.c b/block/ioctl.c
index 4160f4e6bd5b..a2158c5e8e3e 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -394,7 +394,11 @@ static int blkdev_roset(struct block_device *bdev, unsigned cmd,
 		if (ret)
 			return ret;
 	}
-	bdev->bd_read_only = n;
+	if (!bdev->bd_read_only != !n) {
+		bdev->bd_read_only = n;
+		clear_bit(GD_ROWR_WARNED, &bdev->bd_disk->state);
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 51fa7ffdee83..832f7f81e46c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -157,6 +157,7 @@ struct gendisk {
 #define GD_ADDED			4
 #define GD_SUPPRESS_PART_SCAN		5
 #define GD_OWNS_QUEUE			6
+#define GD_ROWR_WARNED			7
 
 	struct mutex open_mutex;	/* open/close mutex */
 	unsigned open_partitions;	/* number of open partitions */
-- 
2.31.1

Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[Yu Kuai]

Hi,

在 2023/11/07 11:50, yebin (H) 写道:
> 
> 
> On 2023/11/7 19:12, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> If one of the underlying disks of raid or dm is set to read-only, then
>> each io will generate new log, which will cause message storm. This
>> environment is indeed problematic, however we can't make sure our
>> naive custormer won't do this, hence use pr_warn_ratelimited() to
>> prevent message storm in this case.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   block/blk-core.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 9d51e9894ece..fdf25b8d6e78 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -501,8 +501,8 @@ static inline void bio_check_ro(struct bio *bio)
>>       if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
>>           if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
>>               return;
>> -        pr_warn("Trying to write to read-only block-device %pg\n",
>> -            bio->bi_bdev);
>> +        pr_warn_ratelimited("Trying to write to read-only 
>> block-device %pg\n",
>> +                    bio->bi_bdev);
> Acctually, before commit  57e95e4670d1 ("block: fix and cleanup 
> bio_check_ro") , there's only print warning once.
> I wrote a patch earlier, set GD_ROWR_WARNED flag for disk after emit 
> warning. You can look at the patch in the
> attachment, Is it possible to solve your problem.

Yes, this can work, other than that I think the flag should be in
block_device instead of gendisk.

However, I'm not sure which is better for now, hope Christoph or Ming
or anyone can give some advise.

Thanks,
Kuai
>>           /* Older lvm-tools actually trigger this */
>>       }
>>   }
> 

[PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

If one of the underlying disks of raid or dm is set to read-only, then
each io will generate new log, which will cause message storm. This
environment is indeed problematic, however we can't make sure our
naive custormer won't do this, hence use pr_warn_ratelimited() to
prevent message storm in this case.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9d51e9894ece..fdf25b8d6e78 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -501,8 +501,8 @@ static inline void bio_check_ro(struct bio *bio)
 	if (op_is_write(bio_op(bio)) && bdev_read_only(bio->bi_bdev)) {
 		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
 			return;
-		pr_warn("Trying to write to read-only block-device %pg\n",
-			bio->bi_bdev);
+		pr_warn_ratelimited("Trying to write to read-only block-device %pg\n",
+				    bio->bi_bdev);
 		/* Older lvm-tools actually trigger this */
 	}
 }
-- 
2.39.2

Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[Jens Axboe]

On Tue, 07 Nov 2023 19:12:47 +0800, Yu Kuai wrote:
> If one of the underlying disks of raid or dm is set to read-only, then
> each io will generate new log, which will cause message storm. This
> environment is indeed problematic, however we can't make sure our
> naive custormer won't do this, hence use pr_warn_ratelimited() to
> prevent message storm in this case.
> 
> 
> [...]

Applied, thanks!

[1/1] blk-core: use pr_warn_ratelimited() in bio_check_ro()
      commit: 1b0a151c10a6d823f033023b9fdd9af72a89591b

Best regards,
-- 
Jens Axboe

Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[Christoph Hellwig]

On Tue, Nov 07, 2023 at 07:12:47PM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> If one of the underlying disks of raid or dm is set to read-only, then
> each io will generate new log, which will cause message storm. This
> environment is indeed problematic, however we can't make sure our
> naive custormer won't do this, hence use pr_warn_ratelimited() to
> prevent message storm in this case.

Reducing the log spam sounds good, and I guess the single warning
would be even better.

That being said, why/how is the underlying device set to read-only?
If there is a good reason we should probably add a holder op to tell
the user about it so that it stop sending writes.

Re: [PATCH] blk-core: use pr_warn_ratelimited() in bio_check_ro()

[Yu Kuai]

Hi,

在 2023/11/08 15:16, Christoph Hellwig 写道:
> On Tue, Nov 07, 2023 at 07:12:47PM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> If one of the underlying disks of raid or dm is set to read-only, then
>> each io will generate new log, which will cause message storm. This
>> environment is indeed problematic, however we can't make sure our
>> naive custormer won't do this, hence use pr_warn_ratelimited() to
>> prevent message storm in this case.
> 
> Reducing the log spam sounds good, and I guess the single warning
> would be even better.

Got it.

Jens, I see that you already apply this version, do you want me to send
a new version to generate single warning for each block_device?
> 
> That being said, why/how is the underlying device set to read-only?
> If there is a good reason we should probably add a holder op to tell
> the user about it so that it stop sending writes.
> 
Our custormer use blockdev --getro to underlying device directly,
they're trying to forbid other users to write underlying device, so
in this case I think set underlying device to read-only is not okay
because it's already write opened, however I'm not sure if we want the
ioctl to fail.

Thanks,
Kuai
> 
> .
>