| Message ID | 20161215140715.12732-3-mhocko@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Dec 16, 2016 at 04:40:41PM +0100, Michal Hocko wrote: > Updated patch after Mike noticed a BUG_ON when KM_NOLOCKDEP is used. > --- > From 1497e713e11639157aef21cae29052cb3dc7ab44 Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > Date: Thu, 15 Dec 2016 13:06:43 +0100 > Subject: [PATCH] xfs: introduce and use KM_NOLOCKDEP to silence reclaim > lockdep false positives > > Now that the page allocator offers __GFP_NOLOCKDEP let's introduce > KM_NOLOCKDEP alias for the xfs allocation APIs. While we are at it > also change KM_NOFS users introduced by b17cb364dbbb ("xfs: fix missing > KM_NOFS tags to keep lockdep happy") and use the new flag for them > instead. There is really no reason to make these allocations contexts > weaker just because of the lockdep which even might not be enabled > in most cases. > Hi Michal, I haven't gone back to fully grok b17cb364dbbb ("xfs: fix missing KM_NOFS tags to keep lockdep happy"), so I'm not really familiar with the original problem. FWIW, there was another KM_NOFS instance added by that commit in xlog_cil_prepare_log_vecs() that is now in xlog_cil_alloc_shadow_bufs(). Perhaps Dave can confirm whether the original issue still applies..? Brian > Changes since v1 > - check for KM_NOLOCKDEP in kmem_flags_convert to not hit sanity BUG_ON > as per Mike Galbraith > > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > fs/xfs/kmem.h | 6 +++++- > fs/xfs/libxfs/xfs_da_btree.c | 4 ++-- > fs/xfs/xfs_buf.c | 2 +- > fs/xfs/xfs_dir2_readdir.c | 2 +- > 4 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h > index 689f746224e7..d5d634ef1f7f 100644 > --- a/fs/xfs/kmem.h > +++ b/fs/xfs/kmem.h > @@ -33,6 +33,7 @@ typedef unsigned __bitwise xfs_km_flags_t; > #define KM_NOFS ((__force xfs_km_flags_t)0x0004u) > #define KM_MAYFAIL ((__force xfs_km_flags_t)0x0008u) > #define KM_ZERO ((__force xfs_km_flags_t)0x0010u) > +#define KM_NOLOCKDEP ((__force xfs_km_flags_t)0x0020u) > > /* > * We use a special process flag to avoid recursive callbacks into > @@ -44,7 +45,7 @@ kmem_flags_convert(xfs_km_flags_t flags) > { > gfp_t lflags; > > - BUG_ON(flags & ~(KM_SLEEP|KM_NOSLEEP|KM_NOFS|KM_MAYFAIL|KM_ZERO)); > + BUG_ON(flags & ~(KM_SLEEP|KM_NOSLEEP|KM_NOFS|KM_MAYFAIL|KM_ZERO|KM_NOLOCKDEP)); > > if (flags & KM_NOSLEEP) { > lflags = GFP_ATOMIC | __GFP_NOWARN; > @@ -57,6 +58,9 @@ kmem_flags_convert(xfs_km_flags_t flags) > if (flags & KM_ZERO) > lflags |= __GFP_ZERO; > > + if (flags & KM_NOLOCKDEP) > + lflags |= __GFP_NOLOCKDEP; > + > return lflags; > } > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index f2dc1a950c85..b8b5f6914863 100644 > --- a/fs/xfs/libxfs/xfs_da_btree.c > +++ b/fs/xfs/libxfs/xfs_da_btree.c > @@ -2429,7 +2429,7 @@ xfs_buf_map_from_irec( > > if (nirecs > 1) { > map = kmem_zalloc(nirecs * sizeof(struct xfs_buf_map), > - KM_SLEEP | KM_NOFS); > + KM_SLEEP | KM_NOLOCKDEP); > if (!map) > return -ENOMEM; > *mapp = map; > @@ -2488,7 +2488,7 @@ xfs_dabuf_map( > */ > if (nfsb != 1) > irecs = kmem_zalloc(sizeof(irec) * nfsb, > - KM_SLEEP | KM_NOFS); > + KM_SLEEP | KM_NOLOCKDEP); > > nirecs = nfsb; > error = xfs_bmapi_read(dp, (xfs_fileoff_t)bno, nfsb, irecs, > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 7f0a01f7b592..f31ae592dcae 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -1785,7 +1785,7 @@ xfs_alloc_buftarg( > { > xfs_buftarg_t *btp; > > - btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); > + btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOLOCKDEP); > > btp->bt_mount = mp; > btp->bt_dev = bdev->bd_dev; > diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c > index 003a99b83bd8..033ed65d7ce6 100644 > --- a/fs/xfs/xfs_dir2_readdir.c > +++ b/fs/xfs/xfs_dir2_readdir.c > @@ -503,7 +503,7 @@ xfs_dir2_leaf_getdents( > length = howmany(bufsize + geo->blksize, (1 << geo->fsblog)); > map_info = kmem_zalloc(offsetof(struct xfs_dir2_leaf_map_info, map) + > (length * sizeof(struct xfs_bmbt_irec)), > - KM_SLEEP | KM_NOFS); > + KM_SLEEP | KM_NOLOCKDEP); > map_info->map_size = length; > > /* > -- > 2.10.2 > > -- > 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 -- 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
On Fri 16-12-16 11:37:50, Brian Foster wrote: > On Fri, Dec 16, 2016 at 04:40:41PM +0100, Michal Hocko wrote: > > Updated patch after Mike noticed a BUG_ON when KM_NOLOCKDEP is used. > > --- > > From 1497e713e11639157aef21cae29052cb3dc7ab44 Mon Sep 17 00:00:00 2001 > > From: Michal Hocko <mhocko@suse.com> > > Date: Thu, 15 Dec 2016 13:06:43 +0100 > > Subject: [PATCH] xfs: introduce and use KM_NOLOCKDEP to silence reclaim > > lockdep false positives > > > > Now that the page allocator offers __GFP_NOLOCKDEP let's introduce > > KM_NOLOCKDEP alias for the xfs allocation APIs. While we are at it > > also change KM_NOFS users introduced by b17cb364dbbb ("xfs: fix missing > > KM_NOFS tags to keep lockdep happy") and use the new flag for them > > instead. There is really no reason to make these allocations contexts > > weaker just because of the lockdep which even might not be enabled > > in most cases. > > > > Hi Michal, > > I haven't gone back to fully grok b17cb364dbbb ("xfs: fix missing > KM_NOFS tags to keep lockdep happy"), so I'm not really familiar with > the original problem. FWIW, there was another KM_NOFS instance added by > that commit in xlog_cil_prepare_log_vecs() that is now in > xlog_cil_alloc_shadow_bufs(). Perhaps Dave can confirm whether the > original issue still applies..? Yes, I've noticed that but the reworked code looked sufficiently different that I didn't dare to simply convert it.
diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h index 689f746224e7..ea3984091d58 100644 --- a/fs/xfs/kmem.h +++ b/fs/xfs/kmem.h @@ -33,6 +33,7 @@ typedef unsigned __bitwise xfs_km_flags_t; #define KM_NOFS ((__force xfs_km_flags_t)0x0004u) #define KM_MAYFAIL ((__force xfs_km_flags_t)0x0008u) #define KM_ZERO ((__force xfs_km_flags_t)0x0010u) +#define KM_NOLOCKDEP ((__force xfs_km_flags_t)0x0020u) /* * We use a special process flag to avoid recursive callbacks into @@ -57,6 +58,9 @@ kmem_flags_convert(xfs_km_flags_t flags) if (flags & KM_ZERO) lflags |= __GFP_ZERO; + if (flags & KM_NOLOCKDEP) + lflags |= __GFP_NOLOCKDEP; + return lflags; } diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c index f2dc1a950c85..b8b5f6914863 100644 --- a/fs/xfs/libxfs/xfs_da_btree.c +++ b/fs/xfs/libxfs/xfs_da_btree.c @@ -2429,7 +2429,7 @@ xfs_buf_map_from_irec( if (nirecs > 1) { map = kmem_zalloc(nirecs * sizeof(struct xfs_buf_map), - KM_SLEEP | KM_NOFS); + KM_SLEEP | KM_NOLOCKDEP); if (!map) return -ENOMEM; *mapp = map; @@ -2488,7 +2488,7 @@ xfs_dabuf_map( */ if (nfsb != 1) irecs = kmem_zalloc(sizeof(irec) * nfsb, - KM_SLEEP | KM_NOFS); + KM_SLEEP | KM_NOLOCKDEP); nirecs = nfsb; error = xfs_bmapi_read(dp, (xfs_fileoff_t)bno, nfsb, irecs, diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 7f0a01f7b592..f31ae592dcae 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1785,7 +1785,7 @@ xfs_alloc_buftarg( { xfs_buftarg_t *btp; - btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOFS); + btp = kmem_zalloc(sizeof(*btp), KM_SLEEP | KM_NOLOCKDEP); btp->bt_mount = mp; btp->bt_dev = bdev->bd_dev; diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c index 003a99b83bd8..033ed65d7ce6 100644 --- a/fs/xfs/xfs_dir2_readdir.c +++ b/fs/xfs/xfs_dir2_readdir.c @@ -503,7 +503,7 @@ xfs_dir2_leaf_getdents( length = howmany(bufsize + geo->blksize, (1 << geo->fsblog)); map_info = kmem_zalloc(offsetof(struct xfs_dir2_leaf_map_info, map) + (length * sizeof(struct xfs_bmbt_irec)), - KM_SLEEP | KM_NOFS); + KM_SLEEP | KM_NOLOCKDEP); map_info->map_size = length; /*