| Message ID | 1515601118-23192-2-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/10/18 9:18 AM, Ilya Dryomov wrote: > Regular block device writes go through blkdev_write_iter(), which does > bdev_read_only(), while zeroout/discard/etc requests are never checked, > both userspace- and kernel-triggered. Add a generic catch-all check to > generic_make_request_checks() to actually enforce ioctl(BLKROSET) and > set_disk_ro(), which is used by quite a few drivers for things like > snapshots, read-only backing files/images, etc. > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me> > --- > block/blk-core.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index f843ae4f858d..d132bec4a266 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors) > return 0; > } > > +static inline bool bio_check_ro(struct bio *bio) > +{ > + struct hd_struct *p; > + bool ret = false; > + > + rcu_read_lock(); > + p = __disk_get_part(bio->bi_disk, bio->bi_partno); > + if (!p || (p->policy && op_is_write(bio_op(bio)))) > + ret = true; > + rcu_read_unlock(); > + > + return ret; > +}> + > static noinline_for_stack bool > generic_make_request_checks(struct bio *bio) > { > @@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio) > goto end_io; > } > > + if (unlikely(bio_check_ro(bio))) { > + printk(KERN_ERR > + "generic_make_request: Trying to write " > + "to read-only block-device %s (partno %d)\n", > + bio_devname(bio, b), bio->bi_partno); > + goto end_io; > + } It's yet another check that adds part lookup and rcu lock/unlock in that path. Can we combine some of them? Make this part of the remap? This overhead impacts every IO, let's not bloat it more than absolutely necessary.
On Wed, Jan 10, 2018 at 5:34 PM, Jens Axboe <axboe@kernel.dk> wrote: > On 1/10/18 9:18 AM, Ilya Dryomov wrote: >> Regular block device writes go through blkdev_write_iter(), which does >> bdev_read_only(), while zeroout/discard/etc requests are never checked, >> both userspace- and kernel-triggered. Add a generic catch-all check to >> generic_make_request_checks() to actually enforce ioctl(BLKROSET) and >> set_disk_ro(), which is used by quite a few drivers for things like >> snapshots, read-only backing files/images, etc. >> >> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> >> Reviewed-by: Sagi Grimberg <sagi@grimberg.me> >> --- >> block/blk-core.c | 23 ++++++++++++++++++++++- >> 1 file changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/block/blk-core.c b/block/blk-core.c >> index f843ae4f858d..d132bec4a266 100644 >> --- a/block/blk-core.c >> +++ b/block/blk-core.c >> @@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors) >> return 0; >> } >> >> +static inline bool bio_check_ro(struct bio *bio) >> +{ >> + struct hd_struct *p; >> + bool ret = false; >> + >> + rcu_read_lock(); >> + p = __disk_get_part(bio->bi_disk, bio->bi_partno); >> + if (!p || (p->policy && op_is_write(bio_op(bio)))) >> + ret = true; >> + rcu_read_unlock(); >> + >> + return ret; >> +}> + >> static noinline_for_stack bool >> generic_make_request_checks(struct bio *bio) >> { >> @@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio) >> goto end_io; >> } >> >> + if (unlikely(bio_check_ro(bio))) { >> + printk(KERN_ERR >> + "generic_make_request: Trying to write " >> + "to read-only block-device %s (partno %d)\n", >> + bio_devname(bio, b), bio->bi_partno); >> + goto end_io; >> + } > > It's yet another check that adds part lookup and rcu lock/unlock in that > path. Can we combine some of them? Make this part of the remap? This > overhead impacts every IO, let's not bloat it more than absolutely > necessary. Yes, combining with should_fail_request check in remap should be easy enough. I considered it, but opted for the less invasive patch. I'll re-spin. Thanks, Ilya
On Wed, Jan 10, 2018 at 09:34:02AM -0700, Jens Axboe wrote: > It's yet another check that adds part lookup and rcu lock/unlock in that > path. Can we combine some of them? Make this part of the remap? This > overhead impacts every IO, let's not bloat it more than absolutely > necessary. Yes, we should be able to integrate this with the partition remap.
diff --git a/block/blk-core.c b/block/blk-core.c index f843ae4f858d..d132bec4a266 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -2123,6 +2123,20 @@ static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors) return 0; } +static inline bool bio_check_ro(struct bio *bio) +{ + struct hd_struct *p; + bool ret = false; + + rcu_read_lock(); + p = __disk_get_part(bio->bi_disk, bio->bi_partno); + if (!p || (p->policy && op_is_write(bio_op(bio)))) + ret = true; + rcu_read_unlock(); + + return ret; +} + static noinline_for_stack bool generic_make_request_checks(struct bio *bio) { @@ -2145,11 +2159,18 @@ generic_make_request_checks(struct bio *bio) goto end_io; } + if (unlikely(bio_check_ro(bio))) { + printk(KERN_ERR + "generic_make_request: Trying to write " + "to read-only block-device %s (partno %d)\n", + bio_devname(bio, b), bio->bi_partno); + goto end_io; + } + /* * For a REQ_NOWAIT based request, return -EOPNOTSUPP * if queue is not a request based queue. */ - if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_rq_based(q)) goto not_supported;