| Message ID | 20240607055912.3586772-10-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3c3e85ddffae93eba1a257eb6939bf5dc1e93b9e |
| Headers | show |
| Series | [01/11] dm-integrity: use the nop integrity profile | expand |
On 6/7/24 07:59, Christoph Hellwig wrote: > Currently registering a checksum-enabled (aka PI) integrity profile sets > the QUEUE_FLAG_STABLE_WRITE flag, and unregistering it clears the flag. > This can incorrectly clear the flag when the driver requires stable > writes even without PI, e.g. in case of iSCSI or NVMe/TCP with data > digest enabled. > > Fix this by looking at the csum_type directly in bdev_stable_writes and > not setting the queue flag. Also remove the blk_queue_stable_writes > helper as the only user in nvme wants to only look at the actual > QUEUE_FLAG_STABLE_WRITE flag as it inherits the integrity configuration > by other means. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-integrity.c | 6 ------ > drivers/nvme/host/multipath.c | 3 ++- > include/linux/blkdev.h | 12 ++++++++---- > 3 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/block/blk-integrity.c b/block/blk-integrity.c > index 1d2d371cd632d3..bec0d1df387ce9 100644 > --- a/block/blk-integrity.c > +++ b/block/blk-integrity.c > @@ -379,9 +379,6 @@ void blk_integrity_register(struct gendisk *disk, struct blk_integrity *template > bi->tag_size = template->tag_size; > bi->pi_offset = template->pi_offset; > > - if (bi->csum_type != BLK_INTEGRITY_CSUM_NONE) > - blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, disk->queue); > - > #ifdef CONFIG_BLK_INLINE_ENCRYPTION > if (disk->queue->crypto_profile) { > pr_warn("blk-integrity: Integrity and hardware inline encryption are not supported together. Disabling hardware inline encryption.\n"); > @@ -404,9 +401,6 @@ void blk_integrity_unregister(struct gendisk *disk) > > if (!bi->tuple_size) > return; > - > - if (bi->csum_type != BLK_INTEGRITY_CSUM_NONE) > - blk_queue_flag_clear(QUEUE_FLAG_STABLE_WRITES, disk->queue); > memset(bi, 0, sizeof(*bi)); > } > EXPORT_SYMBOL(blk_integrity_unregister); > diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c > index d8b6b4648eaff9..12c59db02539e5 100644 > --- a/drivers/nvme/host/multipath.c > +++ b/drivers/nvme/host/multipath.c > @@ -875,7 +875,8 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, __le32 anagrpid) > nvme_mpath_set_live(ns); > } > > - if (blk_queue_stable_writes(ns->queue) && ns->head->disk) > + if (test_bit(QUEUE_FLAG_STABLE_WRITES, &ns->queue->queue_flags) && > + ns->head->disk) > blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, > ns->head->disk->queue); > #ifdef CONFIG_BLK_DEV_ZONED > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index bdd33388e1ced8..f9089750919c6b 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -571,8 +571,6 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q); > #define blk_queue_noxmerges(q) \ > test_bit(QUEUE_FLAG_NOXMERGES, &(q)->queue_flags) > #define blk_queue_nonrot(q) test_bit(QUEUE_FLAG_NONROT, &(q)->queue_flags) > -#define blk_queue_stable_writes(q) \ > - test_bit(QUEUE_FLAG_STABLE_WRITES, &(q)->queue_flags) > #define blk_queue_io_stat(q) test_bit(QUEUE_FLAG_IO_STAT, &(q)->queue_flags) > #define blk_queue_add_random(q) test_bit(QUEUE_FLAG_ADD_RANDOM, &(q)->queue_flags) > #define blk_queue_zone_resetall(q) \ > @@ -1300,8 +1298,14 @@ static inline bool bdev_synchronous(struct block_device *bdev) > > static inline bool bdev_stable_writes(struct block_device *bdev) > { > - return test_bit(QUEUE_FLAG_STABLE_WRITES, > - &bdev_get_queue(bdev)->queue_flags); > + struct request_queue *q = bdev_get_queue(bdev); > + > +#ifdef CONFIG_BLK_DEV_INTEGRITY > + /* BLK_INTEGRITY_CSUM_NONE is not available in blkdev.h */ > + if (q->integrity.csum_type != 0) > + return true; > +#endif > + return test_bit(QUEUE_FLAG_STABLE_WRITES, &q->queue_flags); > } > > static inline bool bdev_write_cache(struct block_device *bdev) Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Christoph, > Currently registering a checksum-enabled (aka PI) integrity profile sets > the QUEUE_FLAG_STABLE_WRITE flag, and unregistering it clears the flag. > This can incorrectly clear the flag when the driver requires stable > writes even without PI, e.g. in case of iSCSI or NVMe/TCP with data > digest enabled. > > Fix this by looking at the csum_type directly in bdev_stable_writes and > not setting the queue flag. Also remove the blk_queue_stable_writes > helper as the only user in nvme wants to only look at the actual > QUEUE_FLAG_STABLE_WRITE flag as it inherits the integrity configuration > by other means. Looks OK. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
diff --git a/block/blk-integrity.c b/block/blk-integrity.c index 1d2d371cd632d3..bec0d1df387ce9 100644 --- a/block/blk-integrity.c +++ b/block/blk-integrity.c @@ -379,9 +379,6 @@ void blk_integrity_register(struct gendisk *disk, struct blk_integrity *template bi->tag_size = template->tag_size; bi->pi_offset = template->pi_offset; - if (bi->csum_type != BLK_INTEGRITY_CSUM_NONE) - blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, disk->queue); - #ifdef CONFIG_BLK_INLINE_ENCRYPTION if (disk->queue->crypto_profile) { pr_warn("blk-integrity: Integrity and hardware inline encryption are not supported together. Disabling hardware inline encryption.\n"); @@ -404,9 +401,6 @@ void blk_integrity_unregister(struct gendisk *disk) if (!bi->tuple_size) return; - - if (bi->csum_type != BLK_INTEGRITY_CSUM_NONE) - blk_queue_flag_clear(QUEUE_FLAG_STABLE_WRITES, disk->queue); memset(bi, 0, sizeof(*bi)); } EXPORT_SYMBOL(blk_integrity_unregister); diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index d8b6b4648eaff9..12c59db02539e5 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -875,7 +875,8 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, __le32 anagrpid) nvme_mpath_set_live(ns); } - if (blk_queue_stable_writes(ns->queue) && ns->head->disk) + if (test_bit(QUEUE_FLAG_STABLE_WRITES, &ns->queue->queue_flags) && + ns->head->disk) blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->head->disk->queue); #ifdef CONFIG_BLK_DEV_ZONED diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index bdd33388e1ced8..f9089750919c6b 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -571,8 +571,6 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q); #define blk_queue_noxmerges(q) \ test_bit(QUEUE_FLAG_NOXMERGES, &(q)->queue_flags) #define blk_queue_nonrot(q) test_bit(QUEUE_FLAG_NONROT, &(q)->queue_flags) -#define blk_queue_stable_writes(q) \ - test_bit(QUEUE_FLAG_STABLE_WRITES, &(q)->queue_flags) #define blk_queue_io_stat(q) test_bit(QUEUE_FLAG_IO_STAT, &(q)->queue_flags) #define blk_queue_add_random(q) test_bit(QUEUE_FLAG_ADD_RANDOM, &(q)->queue_flags) #define blk_queue_zone_resetall(q) \ @@ -1300,8 +1298,14 @@ static inline bool bdev_synchronous(struct block_device *bdev) static inline bool bdev_stable_writes(struct block_device *bdev) { - return test_bit(QUEUE_FLAG_STABLE_WRITES, - &bdev_get_queue(bdev)->queue_flags); + struct request_queue *q = bdev_get_queue(bdev); + +#ifdef CONFIG_BLK_DEV_INTEGRITY + /* BLK_INTEGRITY_CSUM_NONE is not available in blkdev.h */ + if (q->integrity.csum_type != 0) + return true; +#endif + return test_bit(QUEUE_FLAG_STABLE_WRITES, &q->queue_flags); } static inline bool bdev_write_cache(struct block_device *bdev)
Currently registering a checksum-enabled (aka PI) integrity profile sets the QUEUE_FLAG_STABLE_WRITE flag, and unregistering it clears the flag. This can incorrectly clear the flag when the driver requires stable writes even without PI, e.g. in case of iSCSI or NVMe/TCP with data digest enabled. Fix this by looking at the csum_type directly in bdev_stable_writes and not setting the queue flag. Also remove the blk_queue_stable_writes helper as the only user in nvme wants to only look at the actual QUEUE_FLAG_STABLE_WRITE flag as it inherits the integrity configuration by other means. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-integrity.c | 6 ------ drivers/nvme/host/multipath.c | 3 ++- include/linux/blkdev.h | 12 ++++++++---- 3 files changed, 10 insertions(+), 11 deletions(-)