[PATCH] xfs: handle register_shrinker error

[PATCH] xfs: handle register_shrinker error

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

xfs_alloc_buftarg doesn't handle register_shrinker error path. While it
is unlikely to trigger it is not impossible especially with large NUMAs.
Let's handle the failure to make the code more robust.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
this is not tested but it looks quite straightforward. There is one more
unchecked register_shrinker in xfs_qm_init_quotainfo but my absolute
lack of familiarity with the code didn't allow me to come up with a
mechanical fix like this one.

 fs/xfs/xfs_buf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4db6e8d780f6..dd0e18af990c 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1827,7 +1827,10 @@ xfs_alloc_buftarg(
 	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
 	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
 	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
-	register_shrinker(&btp->bt_shrinker);
+	if (register_shrinker(&btp->bt_shrinker)) {
+		percpu_counter_destroy(&btp->bt_io_count);
+		goto error;
+	}
 	return btp;
 
 error:
-- 
2.15.0

Re: [PATCH] xfs: handle register_shrinker error

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

I can take a stab at the quota one.

Re: [PATCH] xfs: handle register_shrinker error

[Michal Hocko]

On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

> I can take a stab at the quota one.

That would be really great!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xfs: handle register_shrinker error

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> > Looks good,
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Thanks!
> 
> > I can take a stab at the quota one.
> 
> That would be really great!
> 
Again, it does not look good. Since kmem_free() does only kvfree(),
nothing will release memory allocated by list_lru_init().

Re: [PATCH] xfs: handle register_shrinker error

[Michal Hocko]

On Fri 24-11-17 01:01:10, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> > > Looks good,
> > > 
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> > Thanks!
> > 
> > > I can take a stab at the quota one.
> > 
> > That would be really great!
> > 
> Again, it does not look good. Since kmem_free() does only kvfree(),
> nothing will release memory allocated by list_lru_init().

Hmm, you are right. I have (blindly) followed the current code flow
which is wrong as well. The following should do the trick. Should I
split that into two patches?
---
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index dd0e18af990c..4c6e86d861fd 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1815,25 +1815,27 @@ xfs_alloc_buftarg(
 	btp->bt_daxdev = dax_dev;
 
 	if (xfs_setsize_buftarg_early(btp, bdev))
-		goto error;
+		goto error_free;
 
 	if (list_lru_init(&btp->bt_lru))
-		goto error;
+		goto error_free;
 
 	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
-		goto error;
+		goto error_lru;
 
 	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
 	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
 	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
 	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
-	if (register_shrinker(&btp->bt_shrinker)) {
-		percpu_counter_destroy(&btp->bt_io_count);
-		goto error;
-	}
+	if (register_shrinker(&btp->bt_shrinker))
+		goto error_pcpu;
 	return btp;
 
-error:
+error_pcpu:
+	percpu_counter_destroy(&btp->bt_io_count);
+error_lru:
+	list_lru_destroy(&btp->bt_lru);
+error_free:
 	kmem_free(btp);
 	return NULL;
 }

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xfs: handle register_shrinker error

[Tetsuo Handa]

Michal Hocko wrote:
> Hmm, you are right. I have (blindly) followed the current code flow
> which is wrong as well. The following should do the trick. Should I
> split that into two patches?

Well, xfs_alloc_buftarg() needs to be more careful.

xfs_alloc_buftarg(
        struct xfs_mount        *mp,
        struct block_device     *bdev,
        struct dax_device       *dax_dev)
{
        xfs_buftarg_t           *btp;

        btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); // This is GFP_NOFS context. But...

        btp->bt_mount = mp;
        btp->bt_dev =  bdev->bd_dev;
        btp->bt_bdev = bdev;
        btp->bt_daxdev = dax_dev;

        if (xfs_setsize_buftarg_early(btp, bdev))
                goto error;

        if (list_lru_init(&btp->bt_lru)) // This is GFP_KERNEL context.
                goto error;

        if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL)) // This is GFP_KERNEL context.
                goto error; // Need to undo list_lru_init() before kmem_free().

        btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
        btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
        btp->bt_shrinker.seeks = DEFAULT_SEEKS;
        btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
        register_shrinker(&btp->bt_shrinker); // This is GFP_KERNEL context.
        return btp;

error:
        kmem_free(btp);
        return NULL;
}

Re: [PATCH] xfs: handle register_shrinker error

[Michal Hocko]

On Fri 24-11-17 01:17:12, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Hmm, you are right. I have (blindly) followed the current code flow
> > which is wrong as well. The following should do the trick. Should I
> > split that into two patches?
> 
> Well, xfs_alloc_buftarg() needs to be more careful.
[...]
>         btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); // This is GFP_NOFS context. But...
[...]
>         if (list_lru_init(&btp->bt_lru)) // This is GFP_KERNEL context.

this sounds like a separate thing to cleanup or document.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xfs: handle register_shrinker error

[Dave Chinner]

On Thu, Nov 23, 2017 at 05:11:37PM +0100, Michal Hocko wrote:
> On Fri 24-11-17 01:01:10, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> > > > Looks good,
> > > > 
> > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > 
> > > Thanks!
> > > 
> > > > I can take a stab at the quota one.
> > > 
> > > That would be really great!
> > > 
> > Again, it does not look good. Since kmem_free() does only kvfree(),
> > nothing will release memory allocated by list_lru_init().
> 
> Hmm, you are right. I have (blindly) followed the current code flow
> which is wrong as well. The following should do the trick. Should I
> split that into two patches?

One is fine by me - if we're need to backport one fix, then we need
to backport both :/

> ---
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index dd0e18af990c..4c6e86d861fd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1815,25 +1815,27 @@ xfs_alloc_buftarg(
>  	btp->bt_daxdev = dax_dev;
>  
>  	if (xfs_setsize_buftarg_early(btp, bdev))
> -		goto error;
> +		goto error_free;
>  
>  	if (list_lru_init(&btp->bt_lru))
> -		goto error;
> +		goto error_free;
>  
>  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> -		goto error;
> +		goto error_lru;
>  
>  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
>  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
>  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
>  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> -	if (register_shrinker(&btp->bt_shrinker)) {
> -		percpu_counter_destroy(&btp->bt_io_count);
> -		goto error;
> -	}
> +	if (register_shrinker(&btp->bt_shrinker))
> +		goto error_pcpu;
>  	return btp;
>  
> -error:
> +error_pcpu:
> +	percpu_counter_destroy(&btp->bt_io_count);
> +error_lru:
> +	list_lru_destroy(&btp->bt_lru);
> +error_free:
>  	kmem_free(btp);
>  	return NULL;

That should do the trick.

Acked-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2] xfs: handle register_shrinker error

[Michal Hocko]

On Fri 24-11-17 09:00:46, Dave Chinner wrote:
> On Thu, Nov 23, 2017 at 05:11:37PM +0100, Michal Hocko wrote:
> > On Fri 24-11-17 01:01:10, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> > > > > Looks good,
> > > > > 
> > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > 
> > > > Thanks!
> > > > 
> > > > > I can take a stab at the quota one.
> > > > 
> > > > That would be really great!
> > > > 
> > > Again, it does not look good. Since kmem_free() does only kvfree(),
> > > nothing will release memory allocated by list_lru_init().
> > 
> > Hmm, you are right. I have (blindly) followed the current code flow
> > which is wrong as well. The following should do the trick. Should I
> > split that into two patches?
> 
> One is fine by me - if we're need to backport one fix, then we need
> to backport both :/

OK

> > ---
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index dd0e18af990c..4c6e86d861fd 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1815,25 +1815,27 @@ xfs_alloc_buftarg(
> >  	btp->bt_daxdev = dax_dev;
> >  
> >  	if (xfs_setsize_buftarg_early(btp, bdev))
> > -		goto error;
> > +		goto error_free;
> >  
> >  	if (list_lru_init(&btp->bt_lru))
> > -		goto error;
> > +		goto error_free;
> >  
> >  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> > -		goto error;
> > +		goto error_lru;
> >  
> >  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
> >  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
> >  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
> >  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> > -	if (register_shrinker(&btp->bt_shrinker)) {
> > -		percpu_counter_destroy(&btp->bt_io_count);
> > -		goto error;
> > -	}
> > +	if (register_shrinker(&btp->bt_shrinker))
> > +		goto error_pcpu;
> >  	return btp;
> >  
> > -error:
> > +error_pcpu:
> > +	percpu_counter_destroy(&btp->bt_io_count);
> > +error_lru:
> > +	list_lru_destroy(&btp->bt_lru);
> > +error_free:
> >  	kmem_free(btp);
> >  	return NULL;
> 
> That should do the trick.
> 
> Acked-by: Dave Chinner <dchinner@redhat.com>

Thanks. Updated patch below
---
>From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 23 Nov 2017 17:13:40 +0100
Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling

percpu_counter_init failure path doesn't clean up &btp->bt_lru list.
Call list_lru_destroy in that error path. Similarly register_shrinker
error path is not handled.

While it is unlikely to trigger these error path, it is not impossible
especially the later might fail with large NUMAs.  Let's handle the
failure to make the code more robust.

Acked-by: Dave Chinner <dchinner@redhat.com>
Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/xfs/xfs_buf.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4db6e8d780f6..4c6e86d861fd 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1815,22 +1815,27 @@ xfs_alloc_buftarg(
 	btp->bt_daxdev = dax_dev;
 
 	if (xfs_setsize_buftarg_early(btp, bdev))
-		goto error;
+		goto error_free;
 
 	if (list_lru_init(&btp->bt_lru))
-		goto error;
+		goto error_free;
 
 	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
-		goto error;
+		goto error_lru;
 
 	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
 	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
 	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
 	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
-	register_shrinker(&btp->bt_shrinker);
+	if (register_shrinker(&btp->bt_shrinker))
+		goto error_pcpu;
 	return btp;
 
-error:
+error_pcpu:
+	percpu_counter_destroy(&btp->bt_io_count);
+error_lru:
+	list_lru_destroy(&btp->bt_lru);
+error_free:
 	kmem_free(btp);
 	return NULL;
 }
-- 
2.15.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

[Tetsuo Handa]

Michal Hocko wrote:
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 23 Nov 2017 17:13:40 +0100
> Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling

Do we need below patch on top of Michal's patch?
KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS
tags to keep lockdep happy"). If not needed, some comment is expected.

---
 fs/xfs/xfs_buf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4c6e86d..b73fc76 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1806,6 +1806,7 @@ struct xfs_buf *
 	struct dax_device	*dax_dev)
 {
 	xfs_buftarg_t		*btp;
+	unsigned int nofs_flag = memalloc_nofs_save();
 
 	btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS);
 
@@ -1829,6 +1830,7 @@ struct xfs_buf *
 	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
 	if (register_shrinker(&btp->bt_shrinker))
 		goto error_pcpu;
+	memalloc_nofs_restore(nofs_flag);
 	return btp;
 
 error_pcpu:
@@ -1837,6 +1839,7 @@ struct xfs_buf *
 	list_lru_destroy(&btp->bt_lru);
 error_free:
 	kmem_free(btp);
+	memalloc_nofs_restore(nofs_flag);
 	return NULL;
 }
 
-- 
1.8.3.1

Re: [PATCH v2] xfs: handle register_shrinker error

[Michal Hocko]

On Fri 24-11-17 21:03:28, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Thanks. Updated patch below
> > ---
> > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Thu, 23 Nov 2017 17:13:40 +0100
> > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> 
> Do we need below patch on top of Michal's patch?
> KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS
> tags to keep lockdep happy"). If not needed, some comment is expected.

I am not not familiar with the code but git blame says that the whole
point of NOFS here was to make lockdep happy. We do have means to
silence the warning if the original concern still applies.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

[Dave Chinner]

On Fri, Nov 24, 2017 at 09:03:28PM +0900, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > Thanks. Updated patch below
> > ---
> > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Thu, 23 Nov 2017 17:13:40 +0100
> > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> 
> Do we need below patch on top of Michal's patch?
> KM_NOFS was added by commit b17cb364dbbbf65a ("xfs: fix missing KM_NOFS
> tags to keep lockdep happy"). If not needed, some comment is expected.

Quite frankly, if the fix is "sprinkle magic undocumented
memalloc_nofs_save() calls around", then you need to think a little
more about the things you just read and the context we're operating
on here.

IOWs:

> ---
>  fs/xfs/xfs_buf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4c6e86d..b73fc76 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1806,6 +1806,7 @@ struct xfs_buf *
>  	struct dax_device	*dax_dev)
>  {
>  	xfs_buftarg_t		*btp;
> +	unsigned int nofs_flag = memalloc_nofs_save();
>  
>  	btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS);

xfs_alloc_buftarg() isn't called from transaction context, so this
KM_NOFS flag wasn't added to prevent reclaim deadlocks - it was
added to avoid stupid lockdep false positives (as was stated in the
commit you quoted).

IOWs, GFP_KERNEL allocations in this function used to trigger
lockdep false positives.

So - think for a minute rather than bashing on the keyboard. Why
aren't the other GFP_KERNEL allocations from this function causing
lockdep to trigger warnings?

Yeah - lockdep is a lot smarter these days and the false positive
trigger has clearly been fixed. i.e. there's no false positive
detection occurring here any more under GFP_KERNEL allocations,
so we don't need the KM_NOFS flag anymore.

IOWs, we don't actually need to touch this code, but if you really
must, just remove the KM_NOFS tag.

-Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs: handle register_shrinker error

[Tetsuo Handa]

Dave Chinner wrote:
> IOWs, we don't actually need to touch this code, but if you really
> must, just remove the KM_NOFS tag.

OK. Then, please remove KM_NOFS. GFP_KERNEL is safer than GFP_NOFS
in the sense that it won't cause OOM lockup due to unable to invoke
the OOM killer.

Re: [PATCH v2] xfs: handle register_shrinker error

[Michal Hocko]

On Sun 26-11-17 11:14:25, Tetsuo Handa wrote:
> Dave Chinner wrote:
> > IOWs, we don't actually need to touch this code, but if you really
> > must, just remove the KM_NOFS tag.
> 
> OK. Then, please remove KM_NOFS. GFP_KERNEL is safer than GFP_NOFS
> in the sense that it won't cause OOM lockup due to unable to invoke
> the OOM killer.

I agree that we should remove nofs request if they are not really
needed. But arguing your usual OOM lockup is (again) over exaggerating.
As a rule of thumb, it is almost always better to have the full reclaim
context rather than reduced one because the later one can influence
other parts of the system as they might need to do more work.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

[Darrick J. Wong]

On Fri, Nov 24, 2017 at 08:39:57AM +0100, Michal Hocko wrote:
> On Fri 24-11-17 09:00:46, Dave Chinner wrote:
> > On Thu, Nov 23, 2017 at 05:11:37PM +0100, Michal Hocko wrote:
> > > On Fri 24-11-17 01:01:10, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > On Thu 23-11-17 05:26:33, Christoph Hellwig wrote:
> > > > > > Looks good,
> > > > > > 
> > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > > > 
> > > > > Thanks!
> > > > > 
> > > > > > I can take a stab at the quota one.
> > > > > 
> > > > > That would be really great!
> > > > > 
> > > > Again, it does not look good. Since kmem_free() does only kvfree(),
> > > > nothing will release memory allocated by list_lru_init().
> > > 
> > > Hmm, you are right. I have (blindly) followed the current code flow
> > > which is wrong as well. The following should do the trick. Should I
> > > split that into two patches?
> > 
> > One is fine by me - if we're need to backport one fix, then we need
> > to backport both :/
> 
> OK
> 
> > > ---
> > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > > index dd0e18af990c..4c6e86d861fd 100644
> > > --- a/fs/xfs/xfs_buf.c
> > > +++ b/fs/xfs/xfs_buf.c
> > > @@ -1815,25 +1815,27 @@ xfs_alloc_buftarg(
> > >  	btp->bt_daxdev = dax_dev;
> > >  
> > >  	if (xfs_setsize_buftarg_early(btp, bdev))
> > > -		goto error;
> > > +		goto error_free;
> > >  
> > >  	if (list_lru_init(&btp->bt_lru))
> > > -		goto error;
> > > +		goto error_free;
> > >  
> > >  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> > > -		goto error;
> > > +		goto error_lru;
> > >  
> > >  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
> > >  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
> > >  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
> > >  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> > > -	if (register_shrinker(&btp->bt_shrinker)) {
> > > -		percpu_counter_destroy(&btp->bt_io_count);
> > > -		goto error;
> > > -	}
> > > +	if (register_shrinker(&btp->bt_shrinker))
> > > +		goto error_pcpu;
> > >  	return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > +	percpu_counter_destroy(&btp->bt_io_count);
> > > +error_lru:
> > > +	list_lru_destroy(&btp->bt_lru);
> > > +error_free:
> > >  	kmem_free(btp);
> > >  	return NULL;
> > 
> > That should do the trick.
> > 
> > Acked-by: Dave Chinner <dchinner@redhat.com>
> 
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 23 Nov 2017 17:13:40 +0100
> Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> 
> percpu_counter_init failure path doesn't clean up &btp->bt_lru list.
> Call list_lru_destroy in that error path. Similarly register_shrinker
> error path is not handled.
> 
> While it is unlikely to trigger these error path, it is not impossible
> especially the later might fail with large NUMAs.  Let's handle the
> failure to make the code more robust.
> 
> Acked-by: Dave Chinner <dchinner@redhat.com>
> Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  fs/xfs/xfs_buf.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 4db6e8d780f6..4c6e86d861fd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg(
>  	btp->bt_daxdev = dax_dev;
>  
>  	if (xfs_setsize_buftarg_early(btp, bdev))
> -		goto error;
> +		goto error_free;
>  
>  	if (list_lru_init(&btp->bt_lru))
> -		goto error;
> +		goto error_free;
>  
>  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> -		goto error;
> +		goto error_lru;
>  
>  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
>  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
>  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
>  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> -	register_shrinker(&btp->bt_shrinker);
> +	if (register_shrinker(&btp->bt_shrinker))
> +		goto error_pcpu;
>  	return btp;
>  
> -error:
> +error_pcpu:
> +	percpu_counter_destroy(&btp->bt_io_count);
> +error_lru:
> +	list_lru_destroy(&btp->bt_lru);
> +error_free:
>  	kmem_free(btp);
>  	return NULL;

This part looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

>  }
> -- 
> 2.15.0
> 
> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: handle register_shrinker error

[Michal Hocko]

On Mon 27-11-17 09:44:53, Darrick J. Wong wrote:
[...]
> > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.com>
> > Date: Thu, 23 Nov 2017 17:13:40 +0100
> > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> > 
> > percpu_counter_init failure path doesn't clean up &btp->bt_lru list.
> > Call list_lru_destroy in that error path. Similarly register_shrinker
> > error path is not handled.
> > 
> > While it is unlikely to trigger these error path, it is not impossible
> > especially the later might fail with large NUMAs.  Let's handle the
> > failure to make the code more robust.
> > 
> > Acked-by: Dave Chinner <dchinner@redhat.com>
> > Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  fs/xfs/xfs_buf.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index 4db6e8d780f6..4c6e86d861fd 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg(
> >  	btp->bt_daxdev = dax_dev;
> >  
> >  	if (xfs_setsize_buftarg_early(btp, bdev))
> > -		goto error;
> > +		goto error_free;
> >  
> >  	if (list_lru_init(&btp->bt_lru))
> > -		goto error;
> > +		goto error_free;
> >  
> >  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> > -		goto error;
> > +		goto error_lru;
> >  
> >  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
> >  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
> >  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
> >  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> > -	register_shrinker(&btp->bt_shrinker);
> > +	if (register_shrinker(&btp->bt_shrinker))
> > +		goto error_pcpu;
> >  	return btp;
> >  
> > -error:
> > +error_pcpu:
> > +	percpu_counter_destroy(&btp->bt_io_count);
> > +error_lru:
> > +	list_lru_destroy(&btp->bt_lru);
> > +error_free:
> >  	kmem_free(btp);
> >  	return NULL;
> 
> This part looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Are you going to apply the patch or should I re-send it with
acks/reviewed-by?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

[Darrick J. Wong]

On Tue, Nov 28, 2017 at 10:35:51AM +0100, Michal Hocko wrote:
> On Mon 27-11-17 09:44:53, Darrick J. Wong wrote:
> [...]
> > > From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@suse.com>
> > > Date: Thu, 23 Nov 2017 17:13:40 +0100
> > > Subject: [PATCH] xfs: fortify xfs_alloc_buftarg error handling
> > > 
> > > percpu_counter_init failure path doesn't clean up &btp->bt_lru list.
> > > Call list_lru_destroy in that error path. Similarly register_shrinker
> > > error path is not handled.
> > > 
> > > While it is unlikely to trigger these error path, it is not impossible
> > > especially the later might fail with large NUMAs.  Let's handle the
> > > failure to make the code more robust.
> > > 
> > > Acked-by: Dave Chinner <dchinner@redhat.com>
> > > Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > ---
> > >  fs/xfs/xfs_buf.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > > index 4db6e8d780f6..4c6e86d861fd 100644
> > > --- a/fs/xfs/xfs_buf.c
> > > +++ b/fs/xfs/xfs_buf.c
> > > @@ -1815,22 +1815,27 @@ xfs_alloc_buftarg(
> > >  	btp->bt_daxdev = dax_dev;
> > >  
> > >  	if (xfs_setsize_buftarg_early(btp, bdev))
> > > -		goto error;
> > > +		goto error_free;
> > >  
> > >  	if (list_lru_init(&btp->bt_lru))
> > > -		goto error;
> > > +		goto error_free;
> > >  
> > >  	if (percpu_counter_init(&btp->bt_io_count, 0, GFP_KERNEL))
> > > -		goto error;
> > > +		goto error_lru;
> > >  
> > >  	btp->bt_shrinker.count_objects = xfs_buftarg_shrink_count;
> > >  	btp->bt_shrinker.scan_objects = xfs_buftarg_shrink_scan;
> > >  	btp->bt_shrinker.seeks = DEFAULT_SEEKS;
> > >  	btp->bt_shrinker.flags = SHRINKER_NUMA_AWARE;
> > > -	register_shrinker(&btp->bt_shrinker);
> > > +	if (register_shrinker(&btp->bt_shrinker))
> > > +		goto error_pcpu;
> > >  	return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > +	percpu_counter_destroy(&btp->bt_io_count);
> > > +error_lru:
> > > +	list_lru_destroy(&btp->bt_lru);
> > > +error_free:
> > >  	kmem_free(btp);
> > >  	return NULL;
> > 
> > This part looks ok,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Are you going to apply the patch or should I re-send it with
> acks/reviewed-by?

I injected all of those when I added the patch to my tree:

Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
Acked-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: handle register_shrinker error

[Michal Hocko]

On Tue 28-11-17 08:39:19, Darrick J. Wong wrote:
> On Tue, Nov 28, 2017 at 10:35:51AM +0100, Michal Hocko wrote:
[...]
> > Are you going to apply the patch or should I re-send it with
> > acks/reviewed-by?
> 
> I injected all of those when I added the patch to my tree:
> 
> Noticed-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> Acked-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks!
-- 
Michal Hocko
SUSE Labs