diff mbox series

block: don't warn when calling fsync on read-only block devices

Message ID alpine.LRH.2.02.1808221217030.30929@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive)
State New, archived
Headers show
Series block: don't warn when calling fsync on read-only block devices | expand

Commit Message

Mikulas Patocka Aug. 22, 2018, 4:23 p.m. UTC
It is possible to call fsync on a read-only handle (for example, fsck.ext2 
does it when doing read-only check), and this call results in kernel 
warning. This bug was introduced by the commit 721c7fc701c7 "block: fail 
op_is_write() requests to read-only partitions".

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 721c7fc701c7 ("block: fail op_is_write() requests to read-only partitions")
Cc: stable@vger.kernel.org	# 4.18

---
 block/blk-core.c              |    3 +++
 drivers/md/dm-verity-target.c |   33 +++++++++++++++++----------------
 2 files changed, 20 insertions(+), 16 deletions(-)

Comments

Jens Axboe Aug. 22, 2018, 4:26 p.m. UTC | #1
On 8/22/18 10:23 AM, Mikulas Patocka wrote:
> It is possible to call fsync on a read-only handle (for example, fsck.ext2 
> does it when doing read-only check), and this call results in kernel 
> warning. This bug was introduced by the commit 721c7fc701c7 "block: fail 
> op_is_write() requests to read-only partitions".

Similar patch is already queued up, it'll go into Linus's tree before
-rc1.
Mikulas Patocka Sept. 3, 2018, 10:05 p.m. UTC | #2
On Wed, 22 Aug 2018, Jens Axboe wrote:

> On 8/22/18 10:23 AM, Mikulas Patocka wrote:
> > It is possible to call fsync on a read-only handle (for example, fsck.ext2 
> > does it when doing read-only check), and this call results in kernel 
> > warning. This bug was introduced by the commit 721c7fc701c7 "block: fail 
> > op_is_write() requests to read-only partitions".
> 
> Similar patch is already queued up, it'll go into Linus's tree before
> -rc1.
> 
> -- 
> Jens Axboe

I think you mean the patch b089cfd95d32638335c551651a8e00fd2c4edb0b, but 
it doesn't work.

The first problem is that bio_op returns the opcode, but op_is_flush 
expects flags as its parameter. bio_op strips off the flags, and so the 
condition op_is_flush is never true and the warning is not shut off.

Anoher problem is that the flags REQ_FUA and REQ_PREFLUSH may be 
superimposed on regular write bio and in that case the warning should be 
triggered. We want to shut the warning off only if bio_sectors is zero 
(i.e. if the flush request is not superimposed on regular write).

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] block: don't warn when doing fsync on read-only devices

It is possible to call fsync on a read-only handle (for example, fsck.ext2
does it when doing read-only check), and this call results in kernel
warning.

The patch b089cfd95d32 ("block: don't warn for flush on read-only device")
attempted to disable the warning, but it is buggy and it doesn't
(op_is_flush tests flags, but bio_op strips off the flags).

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 721c7fc701c7 ("block: fail op_is_write() requests to read-only partitions")
Cc: stable@vger.kernel.org	# 4.18

---
 block/blk-core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c	2018-09-03 23:32:38.550000000 +0200
+++ linux-2.6/block/blk-core.c	2018-09-03 23:33:11.960000000 +0200
@@ -2163,9 +2163,12 @@ static inline bool bio_check_ro(struct b
 {
 	const int op = bio_op(bio);
 
-	if (part->policy && (op_is_write(op) && !op_is_flush(op))) {
+	if (part->policy && op_is_write(op)) {
 		char b[BDEVNAME_SIZE];
 
+		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
+			return false;
+
 		WARN_ONCE(1,
 		       "generic_make_request: Trying to write "
 			"to read-only block-device %s (partno %d)\n",
Jens Axboe Sept. 5, 2018, 10:14 p.m. UTC | #3
On 9/3/18 4:05 PM, Mikulas Patocka wrote:
> 
> 
> On Wed, 22 Aug 2018, Jens Axboe wrote:
> 
>> On 8/22/18 10:23 AM, Mikulas Patocka wrote:
>>> It is possible to call fsync on a read-only handle (for example, fsck.ext2 
>>> does it when doing read-only check), and this call results in kernel 
>>> warning. This bug was introduced by the commit 721c7fc701c7 "block: fail 
>>> op_is_write() requests to read-only partitions".
>>
>> Similar patch is already queued up, it'll go into Linus's tree before
>> -rc1.
>>
>> -- 
>> Jens Axboe
> 
> I think you mean the patch b089cfd95d32638335c551651a8e00fd2c4edb0b, but 
> it doesn't work.
> 
> The first problem is that bio_op returns the opcode, but op_is_flush 
> expects flags as its parameter. bio_op strips off the flags, and so the 
> condition op_is_flush is never true and the warning is not shut off.
> 
> Anoher problem is that the flags REQ_FUA and REQ_PREFLUSH may be 
> superimposed on regular write bio and in that case the warning should be 
> triggered. We want to shut the warning off only if bio_sectors is zero 
> (i.e. if the flush request is not superimposed on regular write).

Thanks for fixing that up, that was somewhat of a disaster. Applied.
diff mbox series

Patch

Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c	2018-08-15 16:47:18.930000000 +0200
+++ linux-2.6/block/blk-core.c	2018-08-22 16:37:19.680000000 +0200
@@ -2155,6 +2155,9 @@  static inline bool bio_check_ro(struct b
 	if (part->policy && op_is_write(bio_op(bio))) {
 		char b[BDEVNAME_SIZE];
 
+		if (op_is_flush(bio->bi_opf) && !bio_sectors(bio))
+			return false;
+
 		WARN_ONCE(1,
 		       "generic_make_request: Trying to write "
 			"to read-only block-device %s (partno %d)\n",