[PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Uday Shankar]

The block layer might merge together discard requests up until the
max_discard_segments limit is hit, but blk_insert_cloned_request checks
the segment count against max_segments regardless of the req op. This
can result in errors like the following when discards are issued through
a DM device and max_discard_segments exceeds max_segments for the queue
of the chosen underlying device.

blk_insert_cloned_request: over max segments limit. (256 > 129)

Fix this by looking at the req_op and enforcing the appropriate segment
limit - max_discard_segments for REQ_OP_DISCARDs and max_segments for
everything else.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
 block/blk-merge.c | 4 +---
 block/blk-mq.c    | 5 +++--
 block/blk.h       | 8 ++++++++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index b7c193d67..7f663c2d3 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -588,9 +588,7 @@ EXPORT_SYMBOL(__blk_rq_map_sg);
 
 static inline unsigned int blk_rq_get_max_segments(struct request *rq)
 {
-	if (req_op(rq) == REQ_OP_DISCARD)
-		return queue_max_discard_segments(rq->q);
-	return queue_max_segments(rq->q);
+	return blk_queue_get_max_segments(rq->q, req_op(rq));
 }
 
 static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
diff --git a/block/blk-mq.c b/block/blk-mq.c
index d3494a796..121b20230 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3000,6 +3000,7 @@ blk_status_t blk_insert_cloned_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
 	unsigned int max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
+	unsigned int max_segments = blk_queue_get_max_segments(q, req_op(rq));
 	blk_status_t ret;
 
 	if (blk_rq_sectors(rq) > max_sectors) {
@@ -3026,9 +3027,9 @@ blk_status_t blk_insert_cloned_request(struct request *rq)
 	 * original queue.
 	 */
 	rq->nr_phys_segments = blk_recalc_rq_segments(rq);
-	if (rq->nr_phys_segments > queue_max_segments(q)) {
+	if (rq->nr_phys_segments > max_segments) {
 		printk(KERN_ERR "%s: over max segments limit. (%hu > %hu)\n",
-			__func__, rq->nr_phys_segments, queue_max_segments(q));
+			__func__, rq->nr_phys_segments, max_segments);
 		return BLK_STS_IOERR;
 	}
 
diff --git a/block/blk.h b/block/blk.h
index f02381405..8d705c13a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -169,6 +169,14 @@ static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 	return q->limits.max_sectors;
 }
 
+static inline unsigned int blk_queue_get_max_segments(struct request_queue *q,
+						      enum req_op op)
+{
+	if (op == REQ_OP_DISCARD)
+		return queue_max_discard_segments(q);
+	return queue_max_segments(q);
+}
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 void blk_flush_integrity(void);
 bool __bio_integrity_endio(struct bio *);

base-commit: 6bea9ac7c6481c09eb2b61d7cd844fc64a526e3e
-- 
2.25.1

Re: [PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Christoph Hellwig]

On Wed, Feb 15, 2023 at 01:15:08PM -0700, Uday Shankar wrote:
> The block layer might merge together discard requests up until the
> max_discard_segments limit is hit, but blk_insert_cloned_request checks
> the segment count against max_segments regardless of the req op. This
> can result in errors like the following when discards are issued through
> a DM device and max_discard_segments exceeds max_segments for the queue
> of the chosen underlying device.
> 
> blk_insert_cloned_request: over max segments limit. (256 > 129)
> 
> Fix this by looking at the req_op and enforcing the appropriate segment
> limit - max_discard_segments for REQ_OP_DISCARDs and max_segments for
> everything else.

I'd just remove the debug check entirely.

Re: [PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Uday Shankar]

On Wed, Feb 15, 2023 at 10:09:36PM -0800, Christoph Hellwig wrote:
> I'd just remove the debug check entirely

Older kernels have these checks in a separate function called
blk_cloned_rq_check_limits, which carries the following comment:

/**
 * blk_cloned_rq_check_limits - Helper function to check a cloned request
 *                              for the new queue limits
 * @q:  the queue
 * @rq: the request being checked
 *
 * Description:
 *    @rq may have been made based on weaker limitations of upper-level queues
 *    in request stacking drivers, and it may violate the limitation of @q.
 *    Since the block layer and the underlying device driver trust @rq
 *    after it is inserted to @q, it should be checked against @q before
 *    the insertion using this generic function.
 *
 *    Request stacking drivers like request-based dm may change the queue
 *    limits when retrying requests on other queues. Those requests need
 *    to be checked against the new queue limits again during dispatch.
 */.

Is this concern no longer relevant?

Re: blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Mike Snitzer]

On Thu, Feb 16 2023 at  2:27P -0500,
Uday Shankar <ushankar@purestorage.com> wrote:

> On Wed, Feb 15, 2023 at 10:09:36PM -0800, Christoph Hellwig wrote:
> > I'd just remove the debug check entirely
> 
> Older kernels have these checks in a separate function called
> blk_cloned_rq_check_limits, which carries the following comment:
> 
> /**
>  * blk_cloned_rq_check_limits - Helper function to check a cloned request
>  *                              for the new queue limits
>  * @q:  the queue
>  * @rq: the request being checked
>  *
>  * Description:
>  *    @rq may have been made based on weaker limitations of upper-level queues
>  *    in request stacking drivers, and it may violate the limitation of @q.
>  *    Since the block layer and the underlying device driver trust @rq
>  *    after it is inserted to @q, it should be checked against @q before
>  *    the insertion using this generic function.
>  *
>  *    Request stacking drivers like request-based dm may change the queue
>  *    limits when retrying requests on other queues. Those requests need
>  *    to be checked against the new queue limits again during dispatch.
>  */.
> 
> Is this concern no longer relevant?

Still relevant.

Re: [PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Christoph Hellwig]

On Thu, Feb 16, 2023 at 12:27:02PM -0700, Uday Shankar wrote:
>  * Description:
>  *    @rq may have been made based on weaker limitations of upper-level queues
>  *    in request stacking drivers, and it may violate the limitation of @q.
>  *    Since the block layer and the underlying device driver trust @rq
>  *    after it is inserted to @q, it should be checked against @q before
>  *    the insertion using this generic function.
>  *
>  *    Request stacking drivers like request-based dm may change the queue
>  *    limits when retrying requests on other queues. Those requests need
>  *    to be checked against the new queue limits again during dispatch.
>  */.
> 
> Is this concern no longer relevant?

The concern is still valid, but it does not refer to the debug check.
It refers to recalculating nr_phys_segments using
blk_recalc_rq_segments, and the fact that any driver using this
interface needs to stack its limits properly.

Re: blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Mike Snitzer]

On Fri, Feb 17 2023 at 11:16P -0500,
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Feb 16, 2023 at 12:27:02PM -0700, Uday Shankar wrote:
> >  * Description:
> >  *    @rq may have been made based on weaker limitations of upper-level queues
> >  *    in request stacking drivers, and it may violate the limitation of @q.
> >  *    Since the block layer and the underlying device driver trust @rq
> >  *    after it is inserted to @q, it should be checked against @q before
> >  *    the insertion using this generic function.
> >  *
> >  *    Request stacking drivers like request-based dm may change the queue
> >  *    limits when retrying requests on other queues. Those requests need
> >  *    to be checked against the new queue limits again during dispatch.
> >  */.
> > 
> > Is this concern no longer relevant?
> 
> The concern is still valid, but it does not refer to the debug check.
> It refers to recalculating nr_phys_segments using
> blk_recalc_rq_segments, and the fact that any driver using this
> interface needs to stack its limits properly.

It is a negative check that you're calling a debug check.

Much easier to address a bug in a driver when seeing this message than
figuring it out after more elaborate hunting. Not seeing the downside
of preserving/fixing given it is a quick limit check. *shrug*

Re: blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[Uday Shankar]

> Much easier to address a bug in a driver when seeing this message than
> figuring it out after more elaborate hunting. Not seeing the downside
> of preserving/fixing given it is a quick limit check. *shrug*

I tend to agree - considering the check has been here for a while, I
don't know if lower levels will have particularly enlightening logging
(if any at all) for these errors.

Re: [PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request

[kernel test robot]

Hi Uday,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 6bea9ac7c6481c09eb2b61d7cd844fc64a526e3e]

url:    https://github.com/intel-lab-lkp/linux/commits/Uday-Shankar/blk-mq-enforce-op-specific-segment-limits-in-blk_insert_cloned_request/20230216-041718
base:   6bea9ac7c6481c09eb2b61d7cd844fc64a526e3e
patch link:    https://lore.kernel.org/r/20230215201507.494152-1-ushankar%40purestorage.com
patch subject: [PATCH] blk-mq: enforce op-specific segment limits in blk_insert_cloned_request
config: i386-randconfig-a004-20230213 (https://download.01.org/0day-ci/archive/20230216/202302162040.FaI25ul2-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2cd958b09698bedea1a5a5f6298f0d25ec522bf9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Uday-Shankar/blk-mq-enforce-op-specific-segment-limits-in-blk_insert_cloned_request/20230216-041718
        git checkout 2cd958b09698bedea1a5a5f6298f0d25ec522bf9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302162040.FaI25ul2-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> block/blk-mq.c:3032:36: warning: format specifies type 'unsigned short' but the argument has type 'unsigned int' [-Wformat]
                           __func__, rq->nr_phys_segments, max_segments);
                                                           ^~~~~~~~~~~~
   include/linux/printk.h:457:60: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                       ~~~    ^~~~~~~~~~~
   include/linux/printk.h:429:19: note: expanded from macro 'printk_index_wrap'
                   _p_func(_fmt, ##__VA_ARGS__);                           \
                           ~~~~    ^~~~~~~~~~~
   1 warning generated.


vim +3032 block/blk-mq.c

  2993	
  2994	#ifdef CONFIG_BLK_MQ_STACKING
  2995	/**
  2996	 * blk_insert_cloned_request - Helper for stacking drivers to submit a request
  2997	 * @rq: the request being queued
  2998	 */
  2999	blk_status_t blk_insert_cloned_request(struct request *rq)
  3000	{
  3001		struct request_queue *q = rq->q;
  3002		unsigned int max_sectors = blk_queue_get_max_sectors(q, req_op(rq));
  3003		unsigned int max_segments = blk_queue_get_max_segments(q, req_op(rq));
  3004		blk_status_t ret;
  3005	
  3006		if (blk_rq_sectors(rq) > max_sectors) {
  3007			/*
  3008			 * SCSI device does not have a good way to return if
  3009			 * Write Same/Zero is actually supported. If a device rejects
  3010			 * a non-read/write command (discard, write same,etc.) the
  3011			 * low-level device driver will set the relevant queue limit to
  3012			 * 0 to prevent blk-lib from issuing more of the offending
  3013			 * operations. Commands queued prior to the queue limit being
  3014			 * reset need to be completed with BLK_STS_NOTSUPP to avoid I/O
  3015			 * errors being propagated to upper layers.
  3016			 */
  3017			if (max_sectors == 0)
  3018				return BLK_STS_NOTSUPP;
  3019	
  3020			printk(KERN_ERR "%s: over max size limit. (%u > %u)\n",
  3021				__func__, blk_rq_sectors(rq), max_sectors);
  3022			return BLK_STS_IOERR;
  3023		}
  3024	
  3025		/*
  3026		 * The queue settings related to segment counting may differ from the
  3027		 * original queue.
  3028		 */
  3029		rq->nr_phys_segments = blk_recalc_rq_segments(rq);
  3030		if (rq->nr_phys_segments > max_segments) {
  3031			printk(KERN_ERR "%s: over max segments limit. (%hu > %hu)\n",
> 3032				__func__, rq->nr_phys_segments, max_segments);
  3033			return BLK_STS_IOERR;
  3034		}
  3035	
  3036		if (q->disk && should_fail_request(q->disk->part0, blk_rq_bytes(rq)))
  3037			return BLK_STS_IOERR;
  3038	
  3039		if (blk_crypto_insert_cloned_request(rq))
  3040			return BLK_STS_IOERR;
  3041	
  3042		blk_account_io_start(rq);
  3043	
  3044		/*
  3045		 * Since we have a scheduler attached on the top device,
  3046		 * bypass a potential scheduler on the bottom device for
  3047		 * insert.
  3048		 */
  3049		blk_mq_run_dispatch_ops(q,
  3050				ret = blk_mq_request_issue_directly(rq, true));
  3051		if (ret)
  3052			blk_account_io_done(rq, ktime_get_ns());
  3053		return ret;
  3054	}
  3055	EXPORT_SYMBOL_GPL(blk_insert_cloned_request);
  3056	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests