* [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order()
@ 2018-01-19 19:00 Bart Van Assche
2018-01-19 19:06 ` Laurence Oberman
2018-01-19 19:31 ` Jens Axboe
0 siblings, 2 replies; 3+ messages in thread
From: Bart Van Assche @ 2018-01-19 19:00 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Bart Van Assche,
Nicholas A . Bellinger, Laurence Oberman
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 <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Laurence Oberman <loberman@redhat.com>
---
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);
/**
--
2.15.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order()
2018-01-19 19:00 [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order() Bart Van Assche
@ 2018-01-19 19:06 ` Laurence Oberman
2018-01-19 19:31 ` Jens Axboe
1 sibling, 0 replies; 3+ messages in thread
From: Laurence Oberman @ 2018-01-19 19:06 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Nicholas A . Bellinger
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 <loberman@redhat.com>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Laurence Oberman <loberman@redhat.com>
> ---
> 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 <loberman@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order()
2018-01-19 19:00 [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order() Bart Van Assche
2018-01-19 19:06 ` Laurence Oberman
@ 2018-01-19 19:31 ` Jens Axboe
1 sibling, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2018-01-19 19:31 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-block, Christoph Hellwig, Nicholas A . Bellinger,
Laurence Oberman
On 1/19/18 12:00 PM, 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
Applied, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-19 19:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-19 19:00 [PATCH] lib/scatterlist: Fix chaining support in sgl_alloc_order() Bart Van Assche
2018-01-19 19:06 ` Laurence Oberman
2018-01-19 19:31 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox