[PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Bart Van Assche]

This patch on itself does not change the behavior of either ioctl.
However, this patch is necessary to avoid that these ioctls fail
with -EIO if sd_revalidate_disk() is called while these ioctls are
in progress because the current zoned block command code temporarily
clears data that is needed by these ioctls. See also commit
3ed05a987e0f ("blk-zoned: implement ioctls").

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Shaun Tancheff <shaun@tancheff.com>
Cc: stable@vger.kernel.org # v4.10
---
 block/blk-zoned.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 20bfc37e1852..acc71e8c3473 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -127,15 +127,19 @@ int blkdev_report_zones(struct block_device *bdev,
 	if (!q)
 		return -ENXIO;
 
+	blk_queue_enter(q, 0);
+
+	ret = -EOPNOTSUPP;
 	if (!blk_queue_is_zoned(q))
-		return -EOPNOTSUPP;
+		goto exit_queue;
 
+	ret = 0;
 	if (!nrz)
-		return 0;
+		goto exit_queue;
 
 	if (sector > bdev->bd_part->nr_sects) {
 		*nr_zones = 0;
-		return 0;
+		goto exit_queue;
 	}
 
 	/*
@@ -154,9 +158,10 @@ int blkdev_report_zones(struct block_device *bdev,
 	nr_pages = min_t(unsigned int, nr_pages,
 			 queue_max_segments(q));
 
+	ret = -ENOMEM;
 	bio = bio_alloc(gfp_mask, nr_pages);
 	if (!bio)
-		return -ENOMEM;
+		goto exit_queue;
 
 	bio_set_dev(bio, bdev);
 	bio->bi_iter.bi_sector = blk_zone_start(q, sector);
@@ -166,7 +171,7 @@ int blkdev_report_zones(struct block_device *bdev,
 		page = alloc_page(gfp_mask);
 		if (!page) {
 			ret = -ENOMEM;
-			goto out;
+			goto put_bio;
 		}
 		if (!bio_add_page(bio, page, PAGE_SIZE, 0)) {
 			__free_page(page);
@@ -179,7 +184,7 @@ int blkdev_report_zones(struct block_device *bdev,
 	else
 		ret = submit_bio_wait(bio);
 	if (ret)
-		goto out;
+		goto put_bio;
 
 	/*
 	 * Process the report result: skip the header and go through the
@@ -222,11 +227,14 @@ int blkdev_report_zones(struct block_device *bdev,
 	}
 
 	*nr_zones = nz;
-out:
+put_bio:
 	bio_for_each_segment_all(bv, bio, i)
 		__free_page(bv->bv_page);
 	bio_put(bio);
 
+exit_queue:
+	blk_queue_exit(q);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(blkdev_report_zones);
@@ -256,21 +264,25 @@ int blkdev_reset_zones(struct block_device *bdev,
 	if (!q)
 		return -ENXIO;
 
+	blk_queue_enter(q, 0);
+
+	ret = -EOPNOTSUPP;
 	if (!blk_queue_is_zoned(q))
-		return -EOPNOTSUPP;
+		goto out;
 
+	ret = -EINVAL;
 	if (end_sector > bdev->bd_part->nr_sects)
 		/* Out of range */
-		return -EINVAL;
+		goto out;
 
 	/* Check alignment (handle eventual smaller last zone) */
 	zone_sectors = blk_queue_zone_sectors(q);
 	if (sector & (zone_sectors - 1))
-		return -EINVAL;
+		goto out;
 
 	if ((nr_sectors & (zone_sectors - 1)) &&
 	    end_sector != bdev->bd_part->nr_sects)
-		return -EINVAL;
+		goto out;
 
 	while (sector < end_sector) {
 
@@ -283,7 +295,7 @@ int blkdev_reset_zones(struct block_device *bdev,
 		bio_put(bio);
 
 		if (ret)
-			return ret;
+			goto out;
 
 		sector += zone_sectors;
 
@@ -292,7 +304,11 @@ int blkdev_reset_zones(struct block_device *bdev,
 
 	}
 
-	return 0;
+	ret = 0;
+
+out:
+	blk_queue_exit(q);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(blkdev_reset_zones);
 
-- 
2.16.3

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Christoph Hellwig]

On Mon, Apr 16, 2018 at 06:00:34PM -0700, Bart Van Assche wrote:
> This patch on itself does not change the behavior of either ioctl.
> However, this patch is necessary to avoid that these ioctls fail
> with -EIO if sd_revalidate_disk() is called while these ioctls are
> in progress because the current zoned block command code temporarily
> clears data that is needed by these ioctls. See also commit
> 3ed05a987e0f ("blk-zoned: implement ioctls").

Hmm.  I think we need to avoid clearing that data and update it using
RCU instead.  Calling blk_queue_enter before submitting bios is
something that would make zone reporting very different from any
other block layer user.

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Bart Van Assche]

T24gVHVlLCAyMDE4LTA0LTE3IGF0IDE3OjE4ICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gT24gTW9uLCBBcHIgMTYsIDIwMTggYXQgMDY6MDA6MzRQTSAtMDcwMCwgQmFydCBWYW4g
QXNzY2hlIHdyb3RlOg0KPiA+IFRoaXMgcGF0Y2ggb24gaXRzZWxmIGRvZXMgbm90IGNoYW5nZSB0
aGUgYmVoYXZpb3Igb2YgZWl0aGVyIGlvY3RsLg0KPiA+IEhvd2V2ZXIsIHRoaXMgcGF0Y2ggaXMg
bmVjZXNzYXJ5IHRvIGF2b2lkIHRoYXQgdGhlc2UgaW9jdGxzIGZhaWwNCj4gPiB3aXRoIC1FSU8g
aWYgc2RfcmV2YWxpZGF0ZV9kaXNrKCkgaXMgY2FsbGVkIHdoaWxlIHRoZXNlIGlvY3RscyBhcmUN
Cj4gPiBpbiBwcm9ncmVzcyBiZWNhdXNlIHRoZSBjdXJyZW50IHpvbmVkIGJsb2NrIGNvbW1hbmQg
Y29kZSB0ZW1wb3JhcmlseQ0KPiA+IGNsZWFycyBkYXRhIHRoYXQgaXMgbmVlZGVkIGJ5IHRoZXNl
IGlvY3Rscy4gU2VlIGFsc28gY29tbWl0DQo+ID4gM2VkMDVhOTg3ZTBmICgiYmxrLXpvbmVkOiBp
bXBsZW1lbnQgaW9jdGxzIikuDQo+IA0KPiBIbW0uICBJIHRoaW5rIHdlIG5lZWQgdG8gYXZvaWQg
Y2xlYXJpbmcgdGhhdCBkYXRhIGFuZCB1cGRhdGUgaXQgdXNpbmcNCj4gUkNVIGluc3RlYWQuICBD
YWxsaW5nIGJsa19xdWV1ZV9lbnRlciBiZWZvcmUgc3VibWl0dGluZyBiaW9zIGlzDQo+IHNvbWV0
aGluZyB0aGF0IHdvdWxkIG1ha2Ugem9uZSByZXBvcnRpbmcgdmVyeSBkaWZmZXJlbnQgZnJvbSBh
bnkNCj4gb3RoZXIgYmxvY2sgbGF5ZXIgdXNlci4NCg0KSGVsbG8gQ2hyaXN0b3BoLA0KDQpBcyB5
b3Uga25vdyBzb21lIHN0cnVjdCBtZW1iZXJzIHRoYXQgY29udGFpbiB6b25lZCBibG9jayBkZXZp
Y2UgaW5mb3JtYXRpb24NCmFyZSBpbiBzdHJ1Y3QgcmVxdWVzdCBxdWV1ZSBhbmQgb3RoZXJzIGFy
ZSBpbiBzdHJ1Y3Qgc2NzaV9kaXNrOg0KDQpzdHJ1Y3Qgc2NzaV9kaXNrIHsNCglbIC4uLiBdDQoj
aWZkZWYgQ09ORklHX0JMS19ERVZfWk9ORUQNCgl1MzIJCW5yX3pvbmVzOw0KCXUzMgkJem9uZV9i
bG9ja3M7DQoJdTMyCQl6b25lX3NoaWZ0Ow0KCXUzMgkJem9uZXNfb3B0aW1hbF9vcGVuOw0KCXUz
MgkJem9uZXNfb3B0aW1hbF9ub25zZXE7DQoJdTMyCQl6b25lc19tYXhfb3BlbjsNCiNlbmRpZg0K
CVsgLi4uIF0NCn07DQoNCnN0cnVjdCByZXF1ZXN0X3F1ZXVlIHsNCglbIC4uLiBdDQoJdW5zaWdu
ZWQgaW50CQlucl96b25lczsNCgl1bnNpZ25lZCBsb25nCQkqc2VxX3pvbmVzX2JpdG1hcDsNCgl1
bnNpZ25lZCBsb25nCQkqc2VxX3pvbmVzX3dsb2NrOw0KCVsgLi4uIF0NCn07DQoNCkRpZCB5b3Ug
cGVyaGFwcyBtZWFuIHRvIG1vdmUgdGhlc2UgbWVtYmVycyBpbnRvIHR3byBuZXcgc3RydWN0dXJl
cywgdG8gdXNlDQpSQ1UgdG8gdXBkYXRlIHRoZSBwb2ludGVycyB0byB0aGVzZSBzdHJ1Y3R1cmVz
IGFuZCBhbHNvIHRvIHByb3RlY3QgY29kZSB0aGF0DQpyZWFkcyB0aGVzZSBwb2ludGVycyB3aXRo
IHJjdV9yZWFkX2xvY2soKSAvIHJjdV9yZWFkX3VubG9jaygpPyBJZiBzbywgaG93IHRvDQptYWtl
IHN1cmUgdGhhdCBib3RoIHBvaW50ZXJzIGdldCB1cGRhdGVkIHNpbXVsdGFuZW91c2x5IHN1Y2gg
dGhhdCBubyBjb2RlDQpldmVyIHNlZXMgYSBwb2ludGVyIHRvIHRoZSBvbGQgaW5mb3JtYXRpb24g
aW4gZS5nLiBzdHJ1Y3Qgc2NzaV9kaXNrIGFuZCBhDQpwb2ludGVyIHRvIHRoZSBuZXcgaW5mb3Jt
YXRpb24gaW4gc3RydWN0IHJlcXVlc3RfcXVldWU/DQoNClRoYW5rcywNCg0KQmFydC4NCg0KDQo=

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Bart Van Assche]

On Tue, 2018-04-17 at 17:18 +0200, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 06:00:34PM -0700, Bart Van Assche wrote:
> > This patch on itself does not change the behavior of either ioctl.
> > However, this patch is necessary to avoid that these ioctls fail
> > with -EIO if sd_revalidate_disk() is called while these ioctls are
> > in progress because the current zoned block command code temporarily
> > clears data that is needed by these ioctls. See also commit
> > 3ed05a987e0f ("blk-zoned: implement ioctls").
> 
> Hmm.  I think we need to avoid clearing that data and update it using
> RCU instead.  Calling blk_queue_enter before submitting bios is
> something that would make zone reporting very different from any
> other block layer user.

Hello Christoph,

As you know some struct members that contain zoned block device information
are in struct request queue and others are in struct scsi_disk:

struct scsi_disk {
	[ ... ]
#ifdef CONFIG_BLK_DEV_ZONED
	u32		nr_zones;
	u32		zone_blocks;
	u32		zone_shift;
	u32		zones_optimal_open;
	u32		zones_optimal_nonseq;
	u32		zones_max_open;
#endif
	[ ... ]
};

struct request_queue {
	[ ... ]
	unsigned int		nr_zones;
	unsigned long		*seq_zones_bitmap;
	unsigned long		*seq_zones_wlock;
	[ ... ]
};

Did you perhaps mean to move these members into two new structures, to use
RCU to update the pointers to these structures and also to protect code that
reads these pointers with rcu_read_lock() / rcu_read_unlock()? If so, how to
make sure that both pointers get updated simultaneously such that no code
ever sees a pointer to the old information in e.g. struct scsi_disk and a
pointer to the new information in struct request_queue?

Thanks,

Bart.

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Bart Van Assche]

T24gVHVlLCAyMDE4LTA0LTE3IGF0IDE3OjE4ICswMjAwLCBDaHJpc3RvcGggSGVsbHdpZyB3cm90
ZToNCj4gT24gTW9uLCBBcHIgMTYsIDIwMTggYXQgMDY6MDA6MzRQTSAtMDcwMCwgQmFydCBWYW4g
QXNzY2hlIHdyb3RlOg0KPiA+IFRoaXMgcGF0Y2ggb24gaXRzZWxmIGRvZXMgbm90IGNoYW5nZSB0
aGUgYmVoYXZpb3Igb2YgZWl0aGVyIGlvY3RsLg0KPiA+IEhvd2V2ZXIsIHRoaXMgcGF0Y2ggaXMg
bmVjZXNzYXJ5IHRvIGF2b2lkIHRoYXQgdGhlc2UgaW9jdGxzIGZhaWwNCj4gPiB3aXRoIC1FSU8g
aWYgc2RfcmV2YWxpZGF0ZV9kaXNrKCkgaXMgY2FsbGVkIHdoaWxlIHRoZXNlIGlvY3RscyBhcmUN
Cj4gPiBpbiBwcm9ncmVzcyBiZWNhdXNlIHRoZSBjdXJyZW50IHpvbmVkIGJsb2NrIGNvbW1hbmQg
Y29kZSB0ZW1wb3JhcmlseQ0KPiA+IGNsZWFycyBkYXRhIHRoYXQgaXMgbmVlZGVkIGJ5IHRoZXNl
IGlvY3Rscy4gU2VlIGFsc28gY29tbWl0DQo+ID4gM2VkMDVhOTg3ZTBmICgiYmxrLXpvbmVkOiBp
bXBsZW1lbnQgaW9jdGxzIikuDQo+IA0KPiBIbW0uICBJIHRoaW5rIHdlIG5lZWQgdG8gYXZvaWQg
Y2xlYXJpbmcgdGhhdCBkYXRhIGFuZCB1cGRhdGUgaXQgdXNpbmcNCj4gUkNVIGluc3RlYWQuICBD
YWxsaW5nIGJsa19xdWV1ZV9lbnRlciBiZWZvcmUgc3VibWl0dGluZyBiaW9zIGlzDQo+IHNvbWV0
aGluZyB0aGF0IHdvdWxkIG1ha2Ugem9uZSByZXBvcnRpbmcgdmVyeSBkaWZmZXJlbnQgZnJvbSBh
bnkNCj4gb3RoZXIgYmxvY2sgbGF5ZXIgdXNlci4NCg0KSGVsbG8gQ2hyaXN0b3BoLA0KDQpQbGVh
c2UgaGF2ZSBhIGxvb2sgYXQgZ2VuZXJpY19tYWtlX3JlcXVlc3QoKS4gSSB0aGluayB0aGF0IGZ1
bmN0aW9uIHNob3dzDQp0aGF0IGJsa19xdWV1ZV9lbnRlcigpIGlzIGNhbGxlZCBhbnl3YXkgYmVm
b3JlIC5tYWtlX3JlcXVlc3RfZm4oKSBpcyBjYWxsZWQuDQoNClRoYW5rcywNCg0KQmFydC4NCg0K
DQo=

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Bart Van Assche]

On Tue, 2018-04-17 at 17:18 +0200, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 06:00:34PM -0700, Bart Van Assche wrote:
> > This patch on itself does not change the behavior of either ioctl.
> > However, this patch is necessary to avoid that these ioctls fail
> > with -EIO if sd_revalidate_disk() is called while these ioctls are
> > in progress because the current zoned block command code temporarily
> > clears data that is needed by these ioctls. See also commit
> > 3ed05a987e0f ("blk-zoned: implement ioctls").
> 
> Hmm.  I think we need to avoid clearing that data and update it using
> RCU instead.  Calling blk_queue_enter before submitting bios is
> something that would make zone reporting very different from any
> other block layer user.

Hello Christoph,

Please have a look at generic_make_request(). I think that function shows
that blk_queue_enter() is called anyway before .make_request_fn() is called.

Thanks,

Bart.

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[hch]

On Tue, Apr 17, 2018 at 05:35:07PM +0000, Bart Van Assche wrote:
> > Hmm.  I think we need to avoid clearing that data and update it using
> > RCU instead.  Calling blk_queue_enter before submitting bios is
> > something that would make zone reporting very different from any
> > other block layer user.
> 
> Hello Christoph,
> 
> Please have a look at generic_make_request(). I think that function shows
> that blk_queue_enter() is called anyway before .make_request_fn() is called.

Yes, that is the point.  We already call blk_queue_enter in
generic_make_request, which the zone report and reset ops pass through.

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Damien Le Moal]

Q2hyaXN0b3BoLA0KDQpPbiAyMDE4LzA0LzE3IDg6MTgsIENocmlzdG9waCBIZWxsd2lnIHdyb3Rl
Og0KPiBPbiBNb24sIEFwciAxNiwgMjAxOCBhdCAwNjowMDozNFBNIC0wNzAwLCBCYXJ0IFZhbiBB
c3NjaGUgd3JvdGU6DQo+PiBUaGlzIHBhdGNoIG9uIGl0c2VsZiBkb2VzIG5vdCBjaGFuZ2UgdGhl
IGJlaGF2aW9yIG9mIGVpdGhlciBpb2N0bC4NCj4+IEhvd2V2ZXIsIHRoaXMgcGF0Y2ggaXMgbmVj
ZXNzYXJ5IHRvIGF2b2lkIHRoYXQgdGhlc2UgaW9jdGxzIGZhaWwNCj4+IHdpdGggLUVJTyBpZiBz
ZF9yZXZhbGlkYXRlX2Rpc2soKSBpcyBjYWxsZWQgd2hpbGUgdGhlc2UgaW9jdGxzIGFyZQ0KPj4g
aW4gcHJvZ3Jlc3MgYmVjYXVzZSB0aGUgY3VycmVudCB6b25lZCBibG9jayBjb21tYW5kIGNvZGUg
dGVtcG9yYXJpbHkNCj4+IGNsZWFycyBkYXRhIHRoYXQgaXMgbmVlZGVkIGJ5IHRoZXNlIGlvY3Rs
cy4gU2VlIGFsc28gY29tbWl0DQo+PiAzZWQwNWE5ODdlMGYgKCJibGstem9uZWQ6IGltcGxlbWVu
dCBpb2N0bHMiKS4NCj4gDQo+IEhtbS4gIEkgdGhpbmsgd2UgbmVlZCB0byBhdm9pZCBjbGVhcmlu
ZyB0aGF0IGRhdGEgYW5kIHVwZGF0ZSBpdCB1c2luZw0KPiBSQ1UgaW5zdGVhZC4gIENhbGxpbmcg
YmxrX3F1ZXVlX2VudGVyIGJlZm9yZSBzdWJtaXR0aW5nIGJpb3MgaXMNCj4gc29tZXRoaW5nIHRo
YXQgd291bGQgbWFrZSB6b25lIHJlcG9ydGluZyB2ZXJ5IGRpZmZlcmVudCBmcm9tIGFueQ0KPiBv
dGhlciBibG9jayBsYXllciB1c2VyLg0KPiANCg0KV2l0aCB0aGUgc2NzaSBwYXRjaGVzIHRoYXQg
QmFydCBzZW50LCB0aGUgZGF0YSBpcyBub3QgY2xlYXJlZCBhbnltb3JlIHVudGlsIGl0DQppcyBy
ZWFkeSB0byBiZSB1cGRhdGVkIGFsbCBhdCBvbmNlLCBpbiBiZXR3ZWVuIGNhbGxzIHRvIGJsa19t
cV9mcmVlemVfcXVldWUoKQ0KYW5kIGJsa19tcV91bmZyZWV6ZV9xdWV1ZSgpLiBUaGlzIHByZXZl
bnRzIHRoZSBzY2hlZHVsZXIgKGRlYWRsaW5lIGFuZA0KbXEtZGVhZGxpbmUpIGZyb20gdXNpbmcg
cGFydGlhbGx5IGluY29ycmVjdCBkYXRhIHNpbmNlIHRoZSBxdWV1ZSBmcmVlemUNCmd1YXJhbnRl
ZXMgbm8gYWN0aXZpdHkgb24gdGhlIHNjaGVkdWxlciBzaWRlIGR1cmluZyB0aGUgZGF0YSBzd2Fw
Lg0KQnV0IGZvciB0aGUgQklPIGlzc3Vpbmcgc2lkZSwgdGhlIGNhbGxzIHRvIGJsa19xdWV1ZV9l
bnRlcigpIGVuc3VyZXMgdGhlIHNhbWUuDQoNClRoaXMgYWxsIGFsbG93cyB0byBsZWF2ZSB1bm1v
ZGlmaWVkIGFsbCB0aGUgYmxrLXpvbmVkLmMgaGVscGVyIGZ1bmN0aW9ucyB0aGF0DQp1c2UgdGhl
IHJlcXVlc3QgcXVldWUgaW5mb3JtYXRpb24gKGJpdG1hcHMgYW5kIG51bWJlciBvZiB6b25lcyks
IHdpdGhvdXQgYW55DQpuZWVkIGZvciByY3UgbG9jayBjYWxscy4gSSB0aGluayB0aGlzIGlzIGEg
c2ltcGxlciBhcHByb2FjaC4NCg0KQmVzdCByZWdhcmRzLg0KDQotLSANCkRhbWllbiBMZSBNb2Fs
DQpXZXN0ZXJuIERpZ2l0YWwgUmVzZWFyY2g=

Re: [PATCH] block: Avoid executing a report or reset zones while a queue is frozen

[Damien Le Moal]

Christoph,

On 2018/04/17 8:18, Christoph Hellwig wrote:
> On Mon, Apr 16, 2018 at 06:00:34PM -0700, Bart Van Assche wrote:
>> This patch on itself does not change the behavior of either ioctl.
>> However, this patch is necessary to avoid that these ioctls fail
>> with -EIO if sd_revalidate_disk() is called while these ioctls are
>> in progress because the current zoned block command code temporarily
>> clears data that is needed by these ioctls. See also commit
>> 3ed05a987e0f ("blk-zoned: implement ioctls").
> 
> Hmm.  I think we need to avoid clearing that data and update it using
> RCU instead.  Calling blk_queue_enter before submitting bios is
> something that would make zone reporting very different from any
> other block layer user.
> 

With the scsi patches that Bart sent, the data is not cleared anymore until it
is ready to be updated all at once, in between calls to blk_mq_freeze_queue()
and blk_mq_unfreeze_queue(). This prevents the scheduler (deadline and
mq-deadline) from using partially incorrect data since the queue freeze
guarantees no activity on the scheduler side during the data swap.
But for the BIO issuing side, the calls to blk_queue_enter() ensures the same.

This all allows to leave unmodified all the blk-zoned.c helper functions that
use the request queue information (bitmaps and number of zones), without any
need for rcu lock calls. I think this is a simpler approach.

Best regards.

-- 
Damien Le Moal
Western Digital Research