From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Message-ID: <1516388811.17578.1.camel@redhat.com> Subject: Re: [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order() From: Laurence Oberman To: Bart Van Assche , Jens Axboe Cc: linux-block@vger.kernel.org, Christoph Hellwig , "Nicholas A . Bellinger" Date: Fri, 19 Jan 2018 14:06:51 -0500 In-Reply-To: <20180119190054.10789-1-bart.vanassche@wdc.com> References: <20180119190054.10789-1-bart.vanassche@wdc.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: On Fri, 2018-01-19 at 11:00 -0800, Bart Van Assche wrote: > This patch avoids that workloads with large block sizes (megabytes) > can trigger the following call stack with the ib_srpt driver (that > driver is the only driver that chains scatterlists allocated by > sgl_alloc_order()): > > BUG: Bad page state in process kworker/0:1H  pfn:2423a78 > page:fffffb03d08e9e00 count:-3 mapcount:0 mapping:          (null) > index:0x0 > flags: 0x57ffffc0000000() > raw: 0057ffffc0000000 0000000000000000 0000000000000000 > fffffffdffffffff > raw: dead000000000100 dead000000000200 0000000000000000 > 0000000000000000 > page dumped because: nonzero _count > CPU: 0 PID: 733 Comm: kworker/0:1H Tainted: G          I      4.15.0- > rc7.bart+ #1 > Hardware name: HP ProLiant DL380 G7, BIOS P67 08/16/2015 > Workqueue: ib-comp-wq ib_cq_poll_work [ib_core] > Call Trace: >  dump_stack+0x5c/0x83 >  bad_page+0xf5/0x10f >  get_page_from_freelist+0xa46/0x11b0 >  __alloc_pages_nodemask+0x103/0x290 >  sgl_alloc_order+0x101/0x180 >  target_alloc_sgl+0x2c/0x40 [target_core_mod] >  srpt_alloc_rw_ctxs+0x173/0x2d0 [ib_srpt] >  srpt_handle_new_iu+0x61e/0x7f0 [ib_srpt] >  __ib_process_cq+0x55/0xa0 [ib_core] >  ib_cq_poll_work+0x1b/0x60 [ib_core] >  process_one_work+0x141/0x340 >  worker_thread+0x47/0x3e0 >  kthread+0xf5/0x130 >  ret_from_fork+0x1f/0x30 > > Fixes: e80a0af4759a ("lib/scatterlist: Introduce sgl_alloc() and > sgl_free()") > Reported-by: Laurence Oberman > Signed-off-by: Bart Van Assche > Cc: Nicholas A. Bellinger > Cc: Laurence Oberman > --- >  drivers/target/target_core_transport.c |  2 +- >  include/linux/scatterlist.h            |  1 + >  lib/scatterlist.c                      | 32 > +++++++++++++++++++++++++++----- >  3 files changed, 29 insertions(+), 6 deletions(-) > > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c > index a001ba711cca..c03a78ee26cd 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -2300,7 +2300,7 @@ static void target_complete_ok_work(struct > work_struct *work) >   >  void target_free_sgl(struct scatterlist *sgl, int nents) >  { > - sgl_free(sgl); > + sgl_free_n_order(sgl, nents, 0); >  } >  EXPORT_SYMBOL(target_free_sgl); >   > diff --git a/include/linux/scatterlist.h > b/include/linux/scatterlist.h > index b8a7c1d1dbe3..22b2131bcdcd 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -282,6 +282,7 @@ struct scatterlist *sgl_alloc_order(unsigned long > long length, >       gfp_t gfp, unsigned int > *nent_p); >  struct scatterlist *sgl_alloc(unsigned long long length, gfp_t gfp, >         unsigned int *nent_p); > +void sgl_free_n_order(struct scatterlist *sgl, int nents, int > order); >  void sgl_free_order(struct scatterlist *sgl, int order); >  void sgl_free(struct scatterlist *sgl); >  #endif /* CONFIG_SGL_ALLOC */ > diff --git a/lib/scatterlist.c b/lib/scatterlist.c > index 9afc9b432083..53728d391d3a 100644 > --- a/lib/scatterlist.c > +++ b/lib/scatterlist.c > @@ -512,7 +512,7 @@ struct scatterlist *sgl_alloc_order(unsigned long > long length, >   if (!sgl) >   return NULL; >   > - sg_init_table(sgl, nent); > + sg_init_table(sgl, nalloc); >   sg = sgl; >   while (length) { >   elem_len = min_t(u64, length, PAGE_SIZE << order); > @@ -526,7 +526,7 @@ struct scatterlist *sgl_alloc_order(unsigned long > long length, >   length -= elem_len; >   sg = sg_next(sg); >   } > - WARN_ON_ONCE(sg); > + WARN_ONCE(length, "length = %lld\n", length); >   if (nent_p) >   *nent_p = nent; >   return sgl; > @@ -549,22 +549,44 @@ struct scatterlist *sgl_alloc(unsigned long > long length, gfp_t gfp, >  EXPORT_SYMBOL(sgl_alloc); >   >  /** > - * sgl_free_order - free a scatterlist and its pages > + * sgl_free_n_order - free a scatterlist and its pages >   * @sgl: Scatterlist with one or more elements > + * @nents: Maximum number of elements to free >   * @order: Second argument for __free_pages() > + * > + * Notes: > + * - If several scatterlists have been chained and each chain > element is > + *   freed separately then it's essential to set nents correctly to > avoid that a > + *   page would get freed twice. > + * - All pages in a chained scatterlist can be freed at once by > setting @nents > + *   to a high number. >   */ > -void sgl_free_order(struct scatterlist *sgl, int order) > +void sgl_free_n_order(struct scatterlist *sgl, int nents, int order) >  { >   struct scatterlist *sg; >   struct page *page; > + int i; >   > - for (sg = sgl; sg; sg = sg_next(sg)) { > + for_each_sg(sgl, sg, nents, i) { > + if (!sg) > + break; >   page = sg_page(sg); >   if (page) >   __free_pages(page, order); >   } >   kfree(sgl); >  } > +EXPORT_SYMBOL(sgl_free_n_order); > + > +/** > + * sgl_free_order - free a scatterlist and its pages > + * @sgl: Scatterlist with one or more elements > + * @order: Second argument for __free_pages() > + */ > +void sgl_free_order(struct scatterlist *sgl, int order) > +{ > + sgl_free_n_order(sgl, INT_MAX, order); > +} >  EXPORT_SYMBOL(sgl_free_order); >   >  /** Spent a good few days working on this with Bart, The issue was very familiar to me so I know its now fixed. Thank you Bart! Tested-by: Laurence Oberman