diff mbox series

blk-core: use pr_warn_ratelimited() in bio_check_ro()

Message ID 20231107111247.2157820-1-yukuai1@huaweicloud.com
State New
Headers
Series blk-core: use pr_warn_ratelimited() in bio_check_ro() |

Commit Message

Yu Kuai Nov. 7, 2023, 11:12 a.m. UTC 
  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(-)

Comments

Ye Bin Nov. 7, 2023, 3:50 a.m. UTC | #1 
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 */
>   	}
>   }
Yu Kuai Nov. 7, 2023, 9:44 a.m. UTC | #2 
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 */
>>       }
>>   }
>
Jens Axboe Nov. 7, 2023, 3:15 p.m. UTC | #3 
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,
Christoph Hellwig Nov. 8, 2023, 7:16 a.m. UTC | #4 
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.
Yu Kuai Nov. 8, 2023, 8:17 a.m. UTC | #5 
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
> 
> .
>
diff mbox series

Patch

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 */
 	}
 }