Message ID | 1596062878-4238-3-git-send-email-ritika.srivastava@oracle.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] block: Return blk_status_t instead of errno codes | expand |
The concept looks fine, but some of the formatting especially in the comments is strange. Also we should not print the message for this case, but just the real error. Updated version with my suggestions below. Also don't you need a third patch that makes dm-multipath stop sending Write Same/Zeroes command when this happens? --- From c056b0523173f17cd3d8ca77a8cfca4e45fe8cb7 Mon Sep 17 00:00:00 2001 From: Ritika Srivastava <ritika.srivastava@oracle.com> Date: Wed, 29 Jul 2020 15:47:58 -0700 Subject: block: better deal with the delayed not supported case in blk_cloned_rq_check_limits If WRITE_ZERO/WRITE_SAME operation is not supported by the storage, blk_cloned_rq_check_limits() will return IO error which will cause device-mapper to fail the paths. Instead, if the queue limit is set to 0, return BLK_STS_NOTSUPP. BLK_STS_NOTSUPP will be ignored by device-mapper and will not fail the paths. Suggested-by: Martin K. Petersen <martin.petersen@oracle.com> Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com> --- block/blk-core.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/block/blk-core.c b/block/blk-core.c index e04ee2c8da2e95..81b830c24b5b4f 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1296,10 +1296,21 @@ EXPORT_SYMBOL(submit_bio); static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, struct request *rq) { - if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q, req_op(rq))) { + unsigned int max_sectors = blk_queue_get_max_sectors(q, req_op(rq)); + + if (blk_rq_sectors(rq) > max_sectors) { + /* + * At least SCSI device do not have a good way to return if + * Write Same is actually supported. So we first try to issue + * one and if it fails clear the max sectors value on failure. + * If this occurs onthe lower device we need to propagate the + * right error code up. + */ + if (max_sectors == 0) + return BLK_STS_NOTSUPP; + printk(KERN_ERR "%s: over max size limit. (%u > %u)\n", - __func__, blk_rq_sectors(rq), - blk_queue_get_max_sectors(q, req_op(rq))); + __func__, blk_rq_sectors(rq), max_sectors); return BLK_STS_IOERR; } @@ -1326,8 +1337,11 @@ static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, */ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *rq) { - if (blk_cloned_rq_check_limits(q, rq)) - return BLK_STS_IOERR; + blk_status_t ret; + + ret = blk_cloned_rq_check_limits(q, rq); + if (ret != BLK_STS_OK) + return ret; if (rq->rq_disk && should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq)))
Hi Christoph, Thank you for the review. > On Aug 13, 2020, at 11:37 PM, Christoph Hellwig <hch@infradead.org> wrote: > > The concept looks fine, but some of the formatting especially in the > comments is strange. Also we should not print the message for this > case, but just the real error. Updated version with my suggestions > below. Your suggestions look good to me. I will include these changes in updated patch and send it for review. > > Also don't you need a third patch that makes dm-multipath stop sending > Write Same/Zeroes command when this happens? blk-lib takes care of not sending any further Write Zero/Same in blkdev_issue_zeroout(). If max_write_zeroes_sectors is set to 0 later on, no more Write Zero/Same will be sent. > > --- > From c056b0523173f17cd3d8ca77a8cfca4e45fe8cb7 Mon Sep 17 00:00:00 2001 > From: Ritika Srivastava <ritika.srivastava@oracle.com> > Date: Wed, 29 Jul 2020 15:47:58 -0700 > Subject: block: better deal with the delayed not supported case in > blk_cloned_rq_check_limits > > If WRITE_ZERO/WRITE_SAME operation is not supported by the storage, > blk_cloned_rq_check_limits() will return IO error which will cause > device-mapper to fail the paths. > > Instead, if the queue limit is set to 0, return BLK_STS_NOTSUPP. > BLK_STS_NOTSUPP will be ignored by device-mapper and will not fail the > paths. > > Suggested-by: Martin K. Petersen <martin.petersen@oracle.com> > Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com> > --- > block/blk-core.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index e04ee2c8da2e95..81b830c24b5b4f 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -1296,10 +1296,21 @@ EXPORT_SYMBOL(submit_bio); > static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, > struct request *rq) > { > - if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q, req_op(rq))) { > + unsigned int max_sectors = blk_queue_get_max_sectors(q, req_op(rq)); > + > + if (blk_rq_sectors(rq) > max_sectors) { > + /* > + * At least SCSI device do not have a good way to return if > + * Write Same is actually supported. So we first try to issue > + * one and if it fails clear the max sectors value on failure. > + * If this occurs onthe lower device we need to propagate the > + * right error code up. > + */ > + if (max_sectors == 0) > + return BLK_STS_NOTSUPP; > + > printk(KERN_ERR "%s: over max size limit. (%u > %u)\n", > - __func__, blk_rq_sectors(rq), > - blk_queue_get_max_sectors(q, req_op(rq))); > + __func__, blk_rq_sectors(rq), max_sectors); > return BLK_STS_IOERR; > } > > @@ -1326,8 +1337,11 @@ static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, > */ > blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *rq) > { > - if (blk_cloned_rq_check_limits(q, rq)) > - return BLK_STS_IOERR; > + blk_status_t ret; > + > + ret = blk_cloned_rq_check_limits(q, rq); > + if (ret != BLK_STS_OK) > + return ret; > > if (rq->rq_disk && > should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq))) > -- > 2.28.0 >
diff --git a/block/blk-core.c b/block/blk-core.c index d241ab8..a6ebfeb 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1223,10 +1223,24 @@ blk_qc_t submit_bio(struct bio *bio) static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, struct request *rq) { - if (blk_rq_sectors(rq) > blk_queue_get_max_sectors(q, req_op(rq))) { + unsigned int queue_max_sector = blk_queue_get_max_sectors(q, req_op(rq)); + + if (blk_rq_sectors(rq) > queue_max_sector) { printk(KERN_ERR "%s: over max size limit. (%u > %u)\n", - __func__, blk_rq_sectors(rq), - blk_queue_get_max_sectors(q, req_op(rq))); + __func__, blk_rq_sectors(rq), queue_max_sector); + + /* If storage does not support the operation, + * the following SCSI error will be returned. + * Illegal Request + * Invalid command operation code + * + * In turn device will set the corresponding queue limit to 0. + * + * If limit is 0, do not return IO error, + * instead return operation not supported. + */ + if (queue_max_sector == 0) + return BLK_STS_NOTSUPP; return BLK_STS_IOERR; } @@ -1253,8 +1267,10 @@ static blk_status_t blk_cloned_rq_check_limits(struct request_queue *q, */ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *rq) { - if (blk_cloned_rq_check_limits(q, rq)) - return BLK_STS_IOERR; + blk_status_t cloned_limit_check = blk_cloned_rq_check_limits(q, rq); + + if (cloned_limit_check != BLK_STS_OK) + return cloned_limit_check; if (rq->rq_disk && should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq)))
If WRITE_ZERO/WRITE_SAME operation is not supported by the storage, blk_cloned_rq_check_limits() will return IO error which will cause device-mapper to fail the paths. Instead, if the queue limit is set to 0, return BLK_STS_NOTSUPP. BLK_STS_NOTSUPP will be ignored by device-mapper and will not fail the paths. Suggested-by: Martin K. Petersen <martin.petersen@oracle.com> Signed-off-by: Ritika Srivastava <ritika.srivastava@oracle.com> --- block/blk-core.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)