diff mbox series

[PATCHv2,2/3] block: introduce rq_list_move

Message ID 20211227164138.2488066-2-kbusch@kernel.org (mailing list archive)
State New, archived
Headers show
Series [PATCHv2,1/3] block: introduce rq_list_for_each_safe macro | expand

Commit Message

Keith Busch Dec. 27, 2021, 4:41 p.m. UTC 
When iterating a list, a particular request may need to be moved for
special handling. Provide a helper function to achieve that so drivers
don't need to reimplement rqlist manipulation.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/blk-mq.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

kernel test robot Dec. 27, 2021, 6:49 p.m. UTC | #1 
Hi Keith,

I love your patch! Perhaps something to improve:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linux-review/Xie-Yongji/nbd-Don-t-use-workqueue-to-handle-recv-work/20211227-171406 linus/master v5.16-rc7 next-20211224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Keith-Busch/block-introduce-rq_list_for_each_safe-macro/20211228-004304
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20211228/202112280210.F89j103t-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/81098ed2c64adf477eae9c21a4188916e0ef5918
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Keith-Busch/block-introduce-rq_list_for_each_safe-macro/20211228-004304
        git checkout 81098ed2c64adf477eae9c21a4188916e0ef5918
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/um/drivers/ubd_kern.c:27:
>> include/linux/blk-mq.h:227:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     227 | static void inline rq_list_move(struct request **src, struct request **dst,
         | ^~~~~~
--
   In file included from include/linux/blktrace_api.h:5,
                    from block/bfq-iosched.h:9,
                    from block/bfq-cgroup.c:16:
>> include/linux/blk-mq.h:227:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
     227 | static void inline rq_list_move(struct request **src, struct request **dst,
         | ^~~~~~
   block/bfq-cgroup.c:1437:6: warning: no previous prototype for 'bfqg_and_blkg_get' [-Wmissing-prototypes]
    1437 | void bfqg_and_blkg_get(struct bfq_group *bfqg) {}
         |      ^~~~~~~~~~~~~~~~~


vim +/inline +227 include/linux/blk-mq.h

   215	
   216	#define rq_dma_dir(rq) \
   217		(op_is_write(req_op(rq)) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
   218	
   219	/**
   220	 * rq_list_move() - move a struct request from one list to another
   221	 * @src: The source list @rq is currently in
   222	 * @dst: The destination list that @rq will be appended to
   223	 * @rq: The request to move
   224	 * @prv: The request preceding @rq in @src (NULL if @rq is the head)
   225	 * @nxt: The request following @rq in @src (NULL if @rq is the tail)
   226	 */
 > 227	static void inline rq_list_move(struct request **src, struct request **dst,
   228					struct request *rq, struct request *prv,
   229					struct request *nxt)
   230	{
   231		if (prv)
   232			prv->rq_next = nxt;
   233		else
   234			*src = nxt;
   235		rq_list_add(dst, rq);
   236	}
   237	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Christoph Hellwig Dec. 29, 2021, 5:41 p.m. UTC | #2 
On Mon, Dec 27, 2021 at 08:41:37AM -0800, Keith Busch wrote:
> +/**
> + * rq_list_move() - move a struct request from one list to another
> + * @src: The source list @rq is currently in
> + * @dst: The destination list that @rq will be appended to
> + * @rq: The request to move
> + * @prv: The request preceding @rq in @src (NULL if @rq is the head)
> + * @nxt: The request following @rq in @src (NULL if @rq is the tail)
> + */
> +static void inline rq_list_move(struct request **src, struct request **dst,
> +				struct request *rq, struct request *prv,
> +				struct request *nxt)
> +{
> +	if (prv)
> +		prv->rq_next = nxt;
> +	else
> +		*src = nxt;
> +	rq_list_add(dst, rq);
> +}

Do we even need the nxt argument?  I think it should always be
rq->rq_next?

Also I'd spell out prev and next for a little more readability.
Keith Busch Dec. 29, 2021, 8:59 p.m. UTC | #3 
On Wed, Dec 29, 2021 at 06:41:09PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 27, 2021 at 08:41:37AM -0800, Keith Busch wrote:
> > +/**
> > + * rq_list_move() - move a struct request from one list to another
> > + * @src: The source list @rq is currently in
> > + * @dst: The destination list that @rq will be appended to
> > + * @rq: The request to move
> > + * @prv: The request preceding @rq in @src (NULL if @rq is the head)
> > + * @nxt: The request following @rq in @src (NULL if @rq is the tail)
> > + */
> > +static void inline rq_list_move(struct request **src, struct request **dst,
> > +				struct request *rq, struct request *prv,
> > +				struct request *nxt)
> > +{
> > +	if (prv)
> > +		prv->rq_next = nxt;
> > +	else
> > +		*src = nxt;
> > +	rq_list_add(dst, rq);
> > +}
> 
> Do we even need the nxt argument?  I think it should always be
> rq->rq_next?

Sure. I only used it here because the safe iterator already has rq_next.
It's not an optimization, so I'll remove it.
diff mbox series

Patch

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 550996cf419c..0efa25abcc1c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -216,6 +216,25 @@  static inline unsigned short req_get_ioprio(struct request *req)
 #define rq_dma_dir(rq) \
 	(op_is_write(req_op(rq)) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
 
+/**
+ * rq_list_move() - move a struct request from one list to another
+ * @src: The source list @rq is currently in
+ * @dst: The destination list that @rq will be appended to
+ * @rq: The request to move
+ * @prv: The request preceding @rq in @src (NULL if @rq is the head)
+ * @nxt: The request following @rq in @src (NULL if @rq is the tail)
+ */
+static void inline rq_list_move(struct request **src, struct request **dst,
+				struct request *rq, struct request *prv,
+				struct request *nxt)
+{
+	if (prv)
+		prv->rq_next = nxt;
+	else
+		*src = nxt;
+	rq_list_add(dst, rq);
+}
+
 enum blk_eh_timer_return {
 	BLK_EH_DONE,		/* drivers has completed the command */
 	BLK_EH_RESET_TIMER,	/* reset timer and try again */