Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-28 Thread 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 
> Signed-off-by: Michal Hocko 
> Acked-by: Dave Chinner 
> Reviewed-by: Darrick J. Wong 
> Signed-off-by: Darrick J. Wong 

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-28 Thread 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 
> Signed-off-by: Michal Hocko 
> Acked-by: Dave Chinner 
> Reviewed-by: Darrick J. Wong 
> Signed-off-by: Darrick J. Wong 

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-28 Thread 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 
> > > 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 >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 
> > > Noticed-by: Tetsuo Handa 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  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(>bt_lru))
> > > - goto error;
> > > + goto error_free;
> > >  
> > >   if (percpu_counter_init(>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(>bt_shrinker);
> > > + if (register_shrinker(>bt_shrinker))
> > > + goto error_pcpu;
> > >   return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > + percpu_counter_destroy(>bt_io_count);
> > > +error_lru:
> > > + list_lru_destroy(>bt_lru);
> > > +error_free:
> > >   kmem_free(btp);
> > >   return NULL;
> > 
> > This part looks ok,
> > Reviewed-by: Darrick J. Wong 
> 
> 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 
Signed-off-by: Michal Hocko 
Acked-by: Dave Chinner 
Reviewed-by: Darrick J. Wong 
Signed-off-by: Darrick J. Wong 

--D

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

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-28 Thread 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 
> > > 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 >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 
> > > Noticed-by: Tetsuo Handa 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  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(>bt_lru))
> > > - goto error;
> > > + goto error_free;
> > >  
> > >   if (percpu_counter_init(>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(>bt_shrinker);
> > > + if (register_shrinker(>bt_shrinker))
> > > + goto error_pcpu;
> > >   return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > + percpu_counter_destroy(>bt_io_count);
> > > +error_lru:
> > > + list_lru_destroy(>bt_lru);
> > > +error_free:
> > >   kmem_free(btp);
> > >   return NULL;
> > 
> > This part looks ok,
> > Reviewed-by: Darrick J. Wong 
> 
> 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 
Signed-off-by: Michal Hocko 
Acked-by: Dave Chinner 
Reviewed-by: Darrick J. Wong 
Signed-off-by: Darrick J. Wong 

--D

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

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-28 Thread 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 
> > 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 >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 
> > Noticed-by: Tetsuo Handa 
> > Signed-off-by: Michal Hocko 
> > ---
> >  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(>bt_lru))
> > -   goto error;
> > +   goto error_free;
> >  
> > if (percpu_counter_init(>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(>bt_shrinker);
> > +   if (register_shrinker(>bt_shrinker))
> > +   goto error_pcpu;
> > return btp;
> >  
> > -error:
> > +error_pcpu:
> > +   percpu_counter_destroy(>bt_io_count);
> > +error_lru:
> > +   list_lru_destroy(>bt_lru);
> > +error_free:
> > kmem_free(btp);
> > return NULL;
> 
> This part looks ok,
> Reviewed-by: Darrick J. Wong 

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

2017-11-28 Thread 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 
> > 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 >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 
> > Noticed-by: Tetsuo Handa 
> > Signed-off-by: Michal Hocko 
> > ---
> >  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(>bt_lru))
> > -   goto error;
> > +   goto error_free;
> >  
> > if (percpu_counter_init(>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(>bt_shrinker);
> > +   if (register_shrinker(>bt_shrinker))
> > +   goto error_pcpu;
> > return btp;
> >  
> > -error:
> > +error_pcpu:
> > +   percpu_counter_destroy(>bt_io_count);
> > +error_lru:
> > +   list_lru_destroy(>bt_lru);
> > +error_free:
> > kmem_free(btp);
> > return NULL;
> 
> This part looks ok,
> Reviewed-by: Darrick J. Wong 

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

2017-11-27 Thread 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 
> > > > > 
> > > > > 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(>bt_lru))
> > > - goto error;
> > > + goto error_free;
> > >  
> > >   if (percpu_counter_init(>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(>bt_shrinker)) {
> > > - percpu_counter_destroy(>bt_io_count);
> > > - goto error;
> > > - }
> > > + if (register_shrinker(>bt_shrinker))
> > > + goto error_pcpu;
> > >   return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > + percpu_counter_destroy(>bt_io_count);
> > > +error_lru:
> > > + list_lru_destroy(>bt_lru);
> > > +error_free:
> > >   kmem_free(btp);
> > >   return NULL;
> > 
> > That should do the trick.
> > 
> > Acked-by: Dave Chinner 
> 
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> 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 >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 
> Noticed-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 
> ---
>  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(>bt_lru))
> - goto error;
> + goto error_free;
>  
>   if (percpu_counter_init(>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(>bt_shrinker);
> + if (register_shrinker(>bt_shrinker))
> + goto error_pcpu;
>   return btp;
>  
> -error:
> +error_pcpu:
> + percpu_counter_destroy(>bt_io_count);
> +error_lru:
> + list_lru_destroy(>bt_lru);
> +error_free:
>   kmem_free(btp);
>   return NULL;

This part looks ok,
Reviewed-by: Darrick J. Wong 

>  }
> -- 
> 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-27 Thread 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 
> > > > > 
> > > > > 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(>bt_lru))
> > > - goto error;
> > > + goto error_free;
> > >  
> > >   if (percpu_counter_init(>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(>bt_shrinker)) {
> > > - percpu_counter_destroy(>bt_io_count);
> > > - goto error;
> > > - }
> > > + if (register_shrinker(>bt_shrinker))
> > > + goto error_pcpu;
> > >   return btp;
> > >  
> > > -error:
> > > +error_pcpu:
> > > + percpu_counter_destroy(>bt_io_count);
> > > +error_lru:
> > > + list_lru_destroy(>bt_lru);
> > > +error_free:
> > >   kmem_free(btp);
> > >   return NULL;
> > 
> > That should do the trick.
> > 
> > Acked-by: Dave Chinner 
> 
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> 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 >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 
> Noticed-by: Tetsuo Handa 
> Signed-off-by: Michal Hocko 
> ---
>  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(>bt_lru))
> - goto error;
> + goto error_free;
>  
>   if (percpu_counter_init(>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(>bt_shrinker);
> + if (register_shrinker(>bt_shrinker))
> + goto error_pcpu;
>   return btp;
>  
> -error:
> +error_pcpu:
> + percpu_counter_destroy(>bt_io_count);
> +error_lru:
> + list_lru_destroy(>bt_lru);
> +error_free:
>   kmem_free(btp);
>   return NULL;

This part looks ok,
Reviewed-by: Darrick J. Wong 

>  }
> -- 
> 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-27 Thread 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

2017-11-27 Thread 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

2017-11-25 Thread 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

2017-11-25 Thread 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

2017-11-25 Thread 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 
> > 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
da...@fromorbit.com

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-25 Thread 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 
> > 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
da...@fromorbit.com

Re: [PATCH v2] xfs: handle register_shrinker error

2017-11-24 Thread 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 
> > 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

2017-11-24 Thread 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 
> > 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

2017-11-24 Thread Tetsuo Handa 
Michal Hocko wrote:
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> 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(>bt_shrinker))
goto error_pcpu;
+   memalloc_nofs_restore(nofs_flag);
return btp;
 
 error_pcpu:
@@ -1837,6 +1839,7 @@ struct xfs_buf *
list_lru_destroy(>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

2017-11-24 Thread Tetsuo Handa 
Michal Hocko wrote:
> Thanks. Updated patch below
> ---
> From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> 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(>bt_shrinker))
goto error_pcpu;
+   memalloc_nofs_restore(nofs_flag);
return btp;
 
 error_pcpu:
@@ -1837,6 +1839,7 @@ struct xfs_buf *
list_lru_destroy(>bt_lru);
 error_free:
kmem_free(btp);
+   memalloc_nofs_restore(nofs_flag);
return NULL;
 }
 
-- 
1.8.3.1

[PATCH v2] xfs: handle register_shrinker error

2017-11-23 Thread 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 
> > > > 
> > > > 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(>bt_lru))
> > -   goto error;
> > +   goto error_free;
> >  
> > if (percpu_counter_init(>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(>bt_shrinker)) {
> > -   percpu_counter_destroy(>bt_io_count);
> > -   goto error;
> > -   }
> > +   if (register_shrinker(>bt_shrinker))
> > +   goto error_pcpu;
> > return btp;
> >  
> > -error:
> > +error_pcpu:
> > +   percpu_counter_destroy(>bt_io_count);
> > +error_lru:
> > +   list_lru_destroy(>bt_lru);
> > +error_free:
> > kmem_free(btp);
> > return NULL;
> 
> That should do the trick.
> 
> Acked-by: Dave Chinner 

Thanks. Updated patch below
---
>From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
From: Michal Hocko 
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 >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 
Noticed-by: Tetsuo Handa 
Signed-off-by: Michal Hocko 
---
 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(>bt_lru))
-   goto error;
+   goto error_free;
 
if (percpu_counter_init(>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(>bt_shrinker);
+   if (register_shrinker(>bt_shrinker))
+   goto error_pcpu;
return btp;
 
-error:
+error_pcpu:
+   percpu_counter_destroy(>bt_io_count);
+error_lru:
+   list_lru_destroy(>bt_lru);
+error_free:
kmem_free(btp);
return NULL;
 }
-- 
2.15.0

-- 
Michal Hocko
SUSE Labs

[PATCH v2] xfs: handle register_shrinker error

2017-11-23 Thread 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 
> > > > 
> > > > 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(>bt_lru))
> > -   goto error;
> > +   goto error_free;
> >  
> > if (percpu_counter_init(>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(>bt_shrinker)) {
> > -   percpu_counter_destroy(>bt_io_count);
> > -   goto error;
> > -   }
> > +   if (register_shrinker(>bt_shrinker))
> > +   goto error_pcpu;
> > return btp;
> >  
> > -error:
> > +error_pcpu:
> > +   percpu_counter_destroy(>bt_io_count);
> > +error_lru:
> > +   list_lru_destroy(>bt_lru);
> > +error_free:
> > kmem_free(btp);
> > return NULL;
> 
> That should do the trick.
> 
> Acked-by: Dave Chinner 

Thanks. Updated patch below
---
>From 1009db61988c48c9a9e327a9d076945b29b02eee Mon Sep 17 00:00:00 2001
From: Michal Hocko 
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 >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 
Noticed-by: Tetsuo Handa 
Signed-off-by: Michal Hocko 
---
 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(>bt_lru))
-   goto error;
+   goto error_free;
 
if (percpu_counter_init(>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(>bt_shrinker);
+   if (register_shrinker(>bt_shrinker))
+   goto error_pcpu;
return btp;
 
-error:
+error_pcpu:
+   percpu_counter_destroy(>bt_io_count);
+error_lru:
+   list_lru_destroy(>bt_lru);
+error_free:
kmem_free(btp);
return NULL;
 }
-- 
2.15.0

-- 
Michal Hocko
SUSE Labs