linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
@ 2021-01-28  6:15 ira.weiny
  2021-02-03 15:56 ` David Sterba
  0 siblings, 1 reply; 6+ messages in thread
From: ira.weiny @ 2021-01-28  6:15 UTC (permalink / raw)
  To: clm, josef, dsterba; +Cc: Ira Weiny, Miao Xie, linux-kernel, linux-fsdevel

From: Ira Weiny <ira.weiny@intel.com>

When a qstripe is required an extra page is allocated and mapped.  There
were 3 problems.

1) There is no reason to map the qstripe page more than 1 time if the
   number of bits set in rbio->dbitmap is greater than one.
2) There is no reason to map the parity page and unmap it each time
   through the loop.
3) There is no corresponding call of kunmap() for the qstripe page.

The page memory can continue to be reused with a single mapping on each
iteration by raid6_call.gen_syndrome() without remapping.  So map the
page for the duration of the loop.

Similarly, improve the algorithm by mapping the parity page just 1 time.

Fixes: 5a6ac9eacb49 ("Btrfs, raid56: support parity scrub on raid56")
To: Chris Mason <clm@fb.com>
To: Josef Bacik <josef@toxicpanda.com>
To: David Sterba <dsterba@suse.com>
Cc: Miao Xie <miaox@cn.fujitsu.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
This was found while replacing kmap() with kmap_local_page().  After
this patch unwinding all the mappings becomes pretty straight forward.

I'm not exactly sure I've worded this commit message intelligently.
Please forgive me if there is a better way to word it.
---
 fs/btrfs/raid56.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 93fbf87bdc8d..b8a39dad0f00 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2363,16 +2363,21 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 	SetPageUptodate(p_page);
 
 	if (has_qstripe) {
+		/* raid6, allocate and map temp space for the qstripe */
 		q_page = alloc_page(GFP_NOFS | __GFP_HIGHMEM);
 		if (!q_page) {
 			__free_page(p_page);
 			goto cleanup;
 		}
 		SetPageUptodate(q_page);
+		pointers[rbio->real_stripes] = kmap(q_page);
 	}
 
 	atomic_set(&rbio->error, 0);
 
+	/* map the parity stripe just once */
+	pointers[nr_data] = kmap(p_page);
+
 	for_each_set_bit(pagenr, rbio->dbitmap, rbio->stripe_npages) {
 		struct page *p;
 		void *parity;
@@ -2382,16 +2387,8 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 			pointers[stripe] = kmap(p);
 		}
 
-		/* then add the parity stripe */
-		pointers[stripe++] = kmap(p_page);
-
 		if (has_qstripe) {
-			/*
-			 * raid6, add the qstripe and call the
-			 * library function to fill in our p/q
-			 */
-			pointers[stripe++] = kmap(q_page);
-
+			/* raid6, call the library function to fill in our p/q */
 			raid6_call.gen_syndrome(rbio->real_stripes, PAGE_SIZE,
 						pointers);
 		} else {
@@ -2412,12 +2409,14 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 
 		for (stripe = 0; stripe < nr_data; stripe++)
 			kunmap(page_in_rbio(rbio, stripe, pagenr, 0));
-		kunmap(p_page);
 	}
 
+	kunmap(p_page);
 	__free_page(p_page);
-	if (q_page)
+	if (q_page) {
+		kunmap(q_page);
 		__free_page(q_page);
+	}
 
 writeback:
 	/*
-- 
2.28.0.rc0.12.gb6a658bd00c9


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
  2021-01-28  6:15 [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing ira.weiny
@ 2021-02-03 15:56 ` David Sterba
  2021-02-04 15:26   ` David Sterba
  0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-02-03 15:56 UTC (permalink / raw)
  To: ira.weiny; +Cc: clm, josef, dsterba, Miao Xie, linux-kernel, linux-fsdevel

On Wed, Jan 27, 2021 at 10:15:03PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> When a qstripe is required an extra page is allocated and mapped.  There
> were 3 problems.
> 
> 1) There is no reason to map the qstripe page more than 1 time if the
>    number of bits set in rbio->dbitmap is greater than one.
> 2) There is no reason to map the parity page and unmap it each time
>    through the loop.
> 3) There is no corresponding call of kunmap() for the qstripe page.
> 
> The page memory can continue to be reused with a single mapping on each
> iteration by raid6_call.gen_syndrome() without remapping.  So map the
> page for the duration of the loop.
> 
> Similarly, improve the algorithm by mapping the parity page just 1 time.
> 
> Fixes: 5a6ac9eacb49 ("Btrfs, raid56: support parity scrub on raid56")
> To: Chris Mason <clm@fb.com>
> To: Josef Bacik <josef@toxicpanda.com>
> To: David Sterba <dsterba@suse.com>
> Cc: Miao Xie <miaox@cn.fujitsu.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> This was found while replacing kmap() with kmap_local_page().  After
> this patch unwinding all the mappings becomes pretty straight forward.
> 
> I'm not exactly sure I've worded this commit message intelligently.
> Please forgive me if there is a better way to word it.

Changelog is good, thanks. I've added stable tags as the missing unmap
is a potential problem.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
  2021-02-03 15:56 ` David Sterba
@ 2021-02-04 15:26   ` David Sterba
  2021-02-05  3:52     ` Ira Weiny
  0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-02-04 15:26 UTC (permalink / raw)
  To: ira.weiny, clm, josef, dsterba, Miao Xie, linux-kernel, linux-fsdevel

On Wed, Feb 03, 2021 at 04:56:48PM +0100, David Sterba wrote:
> On Wed, Jan 27, 2021 at 10:15:03PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > When a qstripe is required an extra page is allocated and mapped.  There
> > were 3 problems.
> > 
> > 1) There is no reason to map the qstripe page more than 1 time if the
> >    number of bits set in rbio->dbitmap is greater than one.
> > 2) There is no reason to map the parity page and unmap it each time
> >    through the loop.
> > 3) There is no corresponding call of kunmap() for the qstripe page.
> > 
> > The page memory can continue to be reused with a single mapping on each
> > iteration by raid6_call.gen_syndrome() without remapping.  So map the
> > page for the duration of the loop.
> > 
> > Similarly, improve the algorithm by mapping the parity page just 1 time.
> > 
> > Fixes: 5a6ac9eacb49 ("Btrfs, raid56: support parity scrub on raid56")
> > To: Chris Mason <clm@fb.com>
> > To: Josef Bacik <josef@toxicpanda.com>
> > To: David Sterba <dsterba@suse.com>
> > Cc: Miao Xie <miaox@cn.fujitsu.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> > ---
> > This was found while replacing kmap() with kmap_local_page().  After
> > this patch unwinding all the mappings becomes pretty straight forward.
> > 
> > I'm not exactly sure I've worded this commit message intelligently.
> > Please forgive me if there is a better way to word it.
> 
> Changelog is good, thanks. I've added stable tags as the missing unmap
> is a potential problem.

There are lots of tests faling, stack traces like below. I haven't seen
anything obvious in the patch so that needs a closer look and for the
time being I can't add the patch to for-next.

 BUG: kernel NULL pointer dereference, address:0000000000000000
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 PGD 0 P4D 0
 Oops: 0002 [#1] PREEMPT SMP
 CPU: 2 PID: 17173 Comm: kworker/u8:5 Not tainted5.11.0-rc6-default+ #1422
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
 Workqueue: btrfs-rmw btrfs_work_helper [btrfs]
 RIP: 0010:raid6_avx22_gen_syndrome+0x103/0x140 [raid6_pq]
 RSP: 0018:ffffa090042cfcf8 EFLAGS: 00010246
 RAX: ffff9e98e1848e80 RBX: ffff9e98d5849000 RCX:0000000000000020
 RDX: ffff9e98e32be000 RSI: 0000000000000000 RDI:ffff9e98e1848e80
 RBP: 0000000000000000 R08: 0000000000000000 R09:0000000000000001
 R10: ffff9e98e1848e90 R11: ffff9e98e1848e98 R12:0000000000001000
 R13: ffff9e98e1848e88 R14: 0000000000000005 R15:0000000000000002
 FS:  0000000000000000(0000) GS:ffff9e993da00000(0000)knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 0000000023143003 CR4:0000000000170ea0
 Call Trace:
  finish_parity_scrub+0x47b/0x7a0 [btrfs]
  raid56_parity_scrub_stripe+0x24e/0x260 [btrfs]
  btrfs_work_helper+0xd5/0x1d0 [btrfs]
  process_one_work+0x262/0x5f0
  worker_thread+0x4e/0x300
  ? process_one_work+0x5f0/0x5f0
  kthread+0x151/0x170
  ? __kthread_bind_mask+0x60/0x60

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
  2021-02-04 15:26   ` David Sterba
@ 2021-02-05  3:52     ` Ira Weiny
  2021-02-05 15:34       ` David Sterba
  0 siblings, 1 reply; 6+ messages in thread
From: Ira Weiny @ 2021-02-05  3:52 UTC (permalink / raw)
  To: dsterba, clm, josef, dsterba, Miao Xie, linux-kernel, linux-fsdevel

On Thu, Feb 04, 2021 at 04:26:08PM +0100, David Sterba wrote:
> On Wed, Feb 03, 2021 at 04:56:48PM +0100, David Sterba wrote:
> > On Wed, Jan 27, 2021 at 10:15:03PM -0800, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > When a qstripe is required an extra page is allocated and mapped.  There
> > > were 3 problems.
> > > 
> > > 1) There is no reason to map the qstripe page more than 1 time if the
> > >    number of bits set in rbio->dbitmap is greater than one.
> > > 2) There is no reason to map the parity page and unmap it each time
> > >    through the loop.
> > > 3) There is no corresponding call of kunmap() for the qstripe page.
> > > 
> > > The page memory can continue to be reused with a single mapping on each
> > > iteration by raid6_call.gen_syndrome() without remapping.  So map the
> > > page for the duration of the loop.
> > > 
> > > Similarly, improve the algorithm by mapping the parity page just 1 time.
> > > 
> > > Fixes: 5a6ac9eacb49 ("Btrfs, raid56: support parity scrub on raid56")
> > > To: Chris Mason <clm@fb.com>
> > > To: Josef Bacik <josef@toxicpanda.com>
> > > To: David Sterba <dsterba@suse.com>
> > > Cc: Miao Xie <miaox@cn.fujitsu.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > ---
> > > This was found while replacing kmap() with kmap_local_page().  After
> > > this patch unwinding all the mappings becomes pretty straight forward.
> > > 
> > > I'm not exactly sure I've worded this commit message intelligently.
> > > Please forgive me if there is a better way to word it.
> > 
> > Changelog is good, thanks. I've added stable tags as the missing unmap
> > is a potential problem.
> 
> There are lots of tests faling, stack traces like below. I haven't seen
> anything obvious in the patch so that needs a closer look and for the
> time being I can't add the patch to for-next.

:-(

I think I may have been off by 1 on the raid6 kmap...

Something like this should fix it...

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index b8a39dad0f00..dbf52f1a379d 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2370,7 +2370,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
                        goto cleanup;
                }
                SetPageUptodate(q_page);
-               pointers[rbio->real_stripes] = kmap(q_page);
+               pointers[rbio->real_stripes - 1] = kmap(q_page);
        }
 
        atomic_set(&rbio->error, 0);

Let me roll a new version.

Sorry,
Ira

> 
>  BUG: kernel NULL pointer dereference, address:0000000000000000
>  #PF: supervisor write access in kernel mode
>  #PF: error_code(0x0002) - not-present page
>  PGD 0 P4D 0
>  Oops: 0002 [#1] PREEMPT SMP
>  CPU: 2 PID: 17173 Comm: kworker/u8:5 Not tainted5.11.0-rc6-default+ #1422
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
>  Workqueue: btrfs-rmw btrfs_work_helper [btrfs]
>  RIP: 0010:raid6_avx22_gen_syndrome+0x103/0x140 [raid6_pq]
>  RSP: 0018:ffffa090042cfcf8 EFLAGS: 00010246
>  RAX: ffff9e98e1848e80 RBX: ffff9e98d5849000 RCX:0000000000000020
>  RDX: ffff9e98e32be000 RSI: 0000000000000000 RDI:ffff9e98e1848e80
>  RBP: 0000000000000000 R08: 0000000000000000 R09:0000000000000001
>  R10: ffff9e98e1848e90 R11: ffff9e98e1848e98 R12:0000000000001000
>  R13: ffff9e98e1848e88 R14: 0000000000000005 R15:0000000000000002
>  FS:  0000000000000000(0000) GS:ffff9e993da00000(0000)knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000000000000 CR3: 0000000023143003 CR4:0000000000170ea0
>  Call Trace:
>   finish_parity_scrub+0x47b/0x7a0 [btrfs]
>   raid56_parity_scrub_stripe+0x24e/0x260 [btrfs]
>   btrfs_work_helper+0xd5/0x1d0 [btrfs]
>   process_one_work+0x262/0x5f0
>   worker_thread+0x4e/0x300
>   ? process_one_work+0x5f0/0x5f0
>   kthread+0x151/0x170
>   ? __kthread_bind_mask+0x60/0x60

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
  2021-02-05  3:52     ` Ira Weiny
@ 2021-02-05 15:34       ` David Sterba
  2021-02-05 16:39         ` Ira Weiny
  0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-02-05 15:34 UTC (permalink / raw)
  To: Ira Weiny
  Cc: dsterba, clm, josef, dsterba, Miao Xie, linux-kernel, linux-fsdevel

On Thu, Feb 04, 2021 at 07:52:36PM -0800, Ira Weiny wrote:
> On Thu, Feb 04, 2021 at 04:26:08PM +0100, David Sterba wrote:
> > On Wed, Feb 03, 2021 at 04:56:48PM +0100, David Sterba wrote:
> > > On Wed, Jan 27, 2021 at 10:15:03PM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > Changelog is good, thanks. I've added stable tags as the missing unmap
> > > is a potential problem.
> > 
> > There are lots of tests faling, stack traces like below. I haven't seen
> > anything obvious in the patch so that needs a closer look and for the
> > time being I can't add the patch to for-next.
> 
> :-(
> 
> I think I may have been off by 1 on the raid6 kmap...
> 
> Something like this should fix it...
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index b8a39dad0f00..dbf52f1a379d 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2370,7 +2370,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
>                         goto cleanup;
>                 }
>                 SetPageUptodate(q_page);
> -               pointers[rbio->real_stripes] = kmap(q_page);
> +               pointers[rbio->real_stripes - 1] = kmap(q_page);

Oh right and tests agree it works.

>         }
>  
>         atomic_set(&rbio->error, 0);
> 
> Let me roll a new version.

No need to, I'll fold the fixup. Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing
  2021-02-05 15:34       ` David Sterba
@ 2021-02-05 16:39         ` Ira Weiny
  0 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2021-02-05 16:39 UTC (permalink / raw)
  To: dsterba, clm, josef, dsterba, Miao Xie, linux-kernel, linux-fsdevel

On Fri, Feb 05, 2021 at 04:34:41PM +0100, David Sterba wrote:
> On Thu, Feb 04, 2021 at 07:52:36PM -0800, Ira Weiny wrote:
> > On Thu, Feb 04, 2021 at 04:26:08PM +0100, David Sterba wrote:
> > > On Wed, Feb 03, 2021 at 04:56:48PM +0100, David Sterba wrote:
> > > > On Wed, Jan 27, 2021 at 10:15:03PM -0800, ira.weiny@intel.com wrote:
> > > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > Changelog is good, thanks. I've added stable tags as the missing unmap
> > > > is a potential problem.
> > > 
> > > There are lots of tests faling, stack traces like below. I haven't seen
> > > anything obvious in the patch so that needs a closer look and for the
> > > time being I can't add the patch to for-next.
> > 
> > :-(
> > 
> > I think I may have been off by 1 on the raid6 kmap...
> > 
> > Something like this should fix it...
> > 
> > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> > index b8a39dad0f00..dbf52f1a379d 100644
> > --- a/fs/btrfs/raid56.c
> > +++ b/fs/btrfs/raid56.c
> > @@ -2370,7 +2370,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
> >                         goto cleanup;
> >                 }
> >                 SetPageUptodate(q_page);
> > -               pointers[rbio->real_stripes] = kmap(q_page);
> > +               pointers[rbio->real_stripes - 1] = kmap(q_page);
> 
> Oh right and tests agree it works.
> 
> >         }
> >  
> >         atomic_set(&rbio->error, 0);
> > 
> > Let me roll a new version.
> 
> No need to, I'll fold the fixup. Thanks.

Oh cool thanks!  :-D
Ira


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-02-05 23:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28  6:15 [PATCH] fs/btrfs: Fix raid6 qstripe kmap'ing ira.weiny
2021-02-03 15:56 ` David Sterba
2021-02-04 15:26   ` David Sterba
2021-02-05  3:52     ` Ira Weiny
2021-02-05 15:34       ` David Sterba
2021-02-05 16:39         ` Ira Weiny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).