| Message ID | 20240821063901.650776-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] xfs: use kfree_rcu_mightsleep to free the perag structures | expand |
On Wed, Aug 21, 2024 at 08:38:29AM +0200, Christoph Hellwig wrote: > The tagged perag helpers are only used in xfs_icache.c in the kernel code > and not at all in xfsprogs. Move them to xfs_icache.c in preparation for > switching to an xarray, for which I have no plan to implement the tagged > lookup functions for userspace. I don't particularly like moving these functions to another file, but I suppose the icache is the only user of these tags. How hard is it to make userspace stubs that assert if anyone ever tries to use it? --D > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_ag.c | 51 ------------------------------------- > fs/xfs/libxfs/xfs_ag.h | 11 -------- > fs/xfs/xfs_icache.c | 58 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+), 62 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > index 4b5a39a83f7aed..87f00f0180846f 100644 > --- a/fs/xfs/libxfs/xfs_ag.c > +++ b/fs/xfs/libxfs/xfs_ag.c > @@ -56,31 +56,6 @@ xfs_perag_get( > return pag; > } > > -/* > - * search from @first to find the next perag with the given tag set. > - */ > -struct xfs_perag * > -xfs_perag_get_tag( > - struct xfs_mount *mp, > - xfs_agnumber_t first, > - unsigned int tag) > -{ > - struct xfs_perag *pag; > - int found; > - > - rcu_read_lock(); > - found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, > - (void **)&pag, first, 1, tag); > - if (found <= 0) { > - rcu_read_unlock(); > - return NULL; > - } > - trace_xfs_perag_get_tag(pag, _RET_IP_); > - atomic_inc(&pag->pag_ref); > - rcu_read_unlock(); > - return pag; > -} > - > /* Get a passive reference to the given perag. */ > struct xfs_perag * > xfs_perag_hold( > @@ -127,32 +102,6 @@ xfs_perag_grab( > return pag; > } > > -/* > - * search from @first to find the next perag with the given tag set. > - */ > -struct xfs_perag * > -xfs_perag_grab_tag( > - struct xfs_mount *mp, > - xfs_agnumber_t first, > - int tag) > -{ > - struct xfs_perag *pag; > - int found; > - > - rcu_read_lock(); > - found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, > - (void **)&pag, first, 1, tag); > - if (found <= 0) { > - rcu_read_unlock(); > - return NULL; > - } > - trace_xfs_perag_grab_tag(pag, _RET_IP_); > - if (!atomic_inc_not_zero(&pag->pag_active_ref)) > - pag = NULL; > - rcu_read_unlock(); > - return pag; > -} > - > void > xfs_perag_rele( > struct xfs_perag *pag) > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h > index d62c266c0b44d5..d9cccd093b60e0 100644 > --- a/fs/xfs/libxfs/xfs_ag.h > +++ b/fs/xfs/libxfs/xfs_ag.h > @@ -153,15 +153,11 @@ void xfs_free_perag(struct xfs_mount *mp); > > /* Passive AG references */ > struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno); > -struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *mp, xfs_agnumber_t agno, > - unsigned int tag); > struct xfs_perag *xfs_perag_hold(struct xfs_perag *pag); > void xfs_perag_put(struct xfs_perag *pag); > > /* Active AG references */ > struct xfs_perag *xfs_perag_grab(struct xfs_mount *, xfs_agnumber_t); > -struct xfs_perag *xfs_perag_grab_tag(struct xfs_mount *, xfs_agnumber_t, > - int tag); > void xfs_perag_rele(struct xfs_perag *pag); > > /* > @@ -263,13 +259,6 @@ xfs_perag_next( > (agno) = 0; \ > for_each_perag_from((mp), (agno), (pag)) > > -#define for_each_perag_tag(mp, agno, pag, tag) \ > - for ((agno) = 0, (pag) = xfs_perag_grab_tag((mp), 0, (tag)); \ > - (pag) != NULL; \ > - (agno) = (pag)->pag_agno + 1, \ > - xfs_perag_rele(pag), \ > - (pag) = xfs_perag_grab_tag((mp), (agno), (tag))) > - > static inline struct xfs_perag * > xfs_perag_next_wrap( > struct xfs_perag *pag, > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index cf629302d48e74..ac604640d36229 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -292,6 +292,64 @@ xfs_perag_clear_inode_tag( > trace_xfs_perag_clear_inode_tag(pag, _RET_IP_); > } > > +/* > + * Search from @first to find the next perag with the given tag set. > + */ > +static struct xfs_perag * > +xfs_perag_get_tag( > + struct xfs_mount *mp, > + xfs_agnumber_t first, > + unsigned int tag) > +{ > + struct xfs_perag *pag; > + int found; > + > + rcu_read_lock(); > + found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, > + (void **)&pag, first, 1, tag); > + if (found <= 0) { > + rcu_read_unlock(); > + return NULL; > + } > + trace_xfs_perag_get_tag(pag, _RET_IP_); > + atomic_inc(&pag->pag_ref); > + rcu_read_unlock(); > + return pag; > +} > + > +/* > + * Search from @first to find the next perag with the given tag set. > + */ > +static struct xfs_perag * > +xfs_perag_grab_tag( > + struct xfs_mount *mp, > + xfs_agnumber_t first, > + int tag) > +{ > + struct xfs_perag *pag; > + int found; > + > + rcu_read_lock(); > + found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, > + (void **)&pag, first, 1, tag); > + if (found <= 0) { > + rcu_read_unlock(); > + return NULL; > + } > + trace_xfs_perag_grab_tag(pag, _RET_IP_); > + if (!atomic_inc_not_zero(&pag->pag_active_ref)) > + pag = NULL; > + rcu_read_unlock(); > + return pag; > +} > + > +#define for_each_perag_tag(mp, agno, pag, tag) \ > + for ((agno) = 0, (pag) = xfs_perag_grab_tag((mp), 0, (tag)); \ > + (pag) != NULL; \ > + (agno) = (pag)->pag_agno + 1, \ > + xfs_perag_rele(pag), \ > + (pag) = xfs_perag_grab_tag((mp), (agno), (tag))) > + > /* > * When we recycle a reclaimable inode, we need to re-initialise the VFS inode > * part of the structure. This is made more complex by the fact we store > -- > 2.43.0 > >
On Wed, Aug 21, 2024 at 09:34:07AM -0700, Darrick J. Wong wrote: > On Wed, Aug 21, 2024 at 08:38:29AM +0200, Christoph Hellwig wrote: > > The tagged perag helpers are only used in xfs_icache.c in the kernel code > > and not at all in xfsprogs. Move them to xfs_icache.c in preparation for > > switching to an xarray, for which I have no plan to implement the tagged > > lookup functions for userspace. > > I don't particularly like moving these functions to another file, but I > suppose the icache is the only user of these tags. How hard is it to > make userspace stubs that assert if anyone ever tries to use it? It might be easier to just implement them in that case like the underlying radix tree ones. But given that they are unused I'd feel rather uncomfortable about it. And more importantly I like to have the function (only one is left by the end) close to the callers as that makes reading and understanding the code easier.
On Wed, Aug 21, 2024 at 09:34:07AM -0700, Darrick J. Wong wrote: > I don't particularly like moving these functions to another file, but I > suppose the icache is the only user of these tags. How hard is it to > make userspace stubs that assert if anyone ever tries to use it? I looked into not moving them, but the annoying thing is that we then need to make the ici_tag_to_mark helper added later and the marks global. Unless this is a blocker for you I'd much prefer to just keep all the tag/mark logic contained in icache.c for now. Things might change a bit if/when we do the generic xfs_group and also use tags for garbage collection of zoned rtgs, but I'd rather build the right abstraction when we get to that. That will probably also include sorting out the current mess with the ICI vs IWALK flags.
On Tue, Aug 27, 2024 at 11:28:48PM -0700, Christoph Hellwig wrote: > On Wed, Aug 21, 2024 at 09:34:07AM -0700, Darrick J. Wong wrote: > > I don't particularly like moving these functions to another file, but I > > suppose the icache is the only user of these tags. How hard is it to > > make userspace stubs that assert if anyone ever tries to use it? > > I looked into not moving them, but the annoying thing is that we then > need to make the ici_tag_to_mark helper added later and the marks > global. Unless this is a blocker for you I'd much prefer to just > keep all the tag/mark logic contained in icache.c for now. Things > might change a bit if/when we do the generic xfs_group and also use > tags for garbage collection of zoned rtgs, but I'd rather build the > right abstraction when we get to that. That will probably also > include sorting out the current mess with the ICI vs IWALK flags. Or converting pag_ici_root to an xarray, and then we can make all of them use the same mark symbols. <shrug> --D
On Wed, Aug 28, 2024 at 09:10:04AM -0700, Darrick J. Wong wrote: > > tags for garbage collection of zoned rtgs, but I'd rather build the > > right abstraction when we get to that. That will probably also > > include sorting out the current mess with the ICI vs IWALK flags. > > Or converting pag_ici_root to an xarray, and then we can make all of > them use the same mark symbols. <shrug> Maybe we could, but someone intentionally separated them out (and than someone, not sure if the same persons were involved, was very sloppy about the separation) so I'll need to look a bit more at the history. And maybe think about a better way of doing this.
diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c index 4b5a39a83f7aed..87f00f0180846f 100644 --- a/fs/xfs/libxfs/xfs_ag.c +++ b/fs/xfs/libxfs/xfs_ag.c @@ -56,31 +56,6 @@ xfs_perag_get( return pag; } -/* - * search from @first to find the next perag with the given tag set. - */ -struct xfs_perag * -xfs_perag_get_tag( - struct xfs_mount *mp, - xfs_agnumber_t first, - unsigned int tag) -{ - struct xfs_perag *pag; - int found; - - rcu_read_lock(); - found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, - (void **)&pag, first, 1, tag); - if (found <= 0) { - rcu_read_unlock(); - return NULL; - } - trace_xfs_perag_get_tag(pag, _RET_IP_); - atomic_inc(&pag->pag_ref); - rcu_read_unlock(); - return pag; -} - /* Get a passive reference to the given perag. */ struct xfs_perag * xfs_perag_hold( @@ -127,32 +102,6 @@ xfs_perag_grab( return pag; } -/* - * search from @first to find the next perag with the given tag set. - */ -struct xfs_perag * -xfs_perag_grab_tag( - struct xfs_mount *mp, - xfs_agnumber_t first, - int tag) -{ - struct xfs_perag *pag; - int found; - - rcu_read_lock(); - found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, - (void **)&pag, first, 1, tag); - if (found <= 0) { - rcu_read_unlock(); - return NULL; - } - trace_xfs_perag_grab_tag(pag, _RET_IP_); - if (!atomic_inc_not_zero(&pag->pag_active_ref)) - pag = NULL; - rcu_read_unlock(); - return pag; -} - void xfs_perag_rele( struct xfs_perag *pag) diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h index d62c266c0b44d5..d9cccd093b60e0 100644 --- a/fs/xfs/libxfs/xfs_ag.h +++ b/fs/xfs/libxfs/xfs_ag.h @@ -153,15 +153,11 @@ void xfs_free_perag(struct xfs_mount *mp); /* Passive AG references */ struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno); -struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *mp, xfs_agnumber_t agno, - unsigned int tag); struct xfs_perag *xfs_perag_hold(struct xfs_perag *pag); void xfs_perag_put(struct xfs_perag *pag); /* Active AG references */ struct xfs_perag *xfs_perag_grab(struct xfs_mount *, xfs_agnumber_t); -struct xfs_perag *xfs_perag_grab_tag(struct xfs_mount *, xfs_agnumber_t, - int tag); void xfs_perag_rele(struct xfs_perag *pag); /* @@ -263,13 +259,6 @@ xfs_perag_next( (agno) = 0; \ for_each_perag_from((mp), (agno), (pag)) -#define for_each_perag_tag(mp, agno, pag, tag) \ - for ((agno) = 0, (pag) = xfs_perag_grab_tag((mp), 0, (tag)); \ - (pag) != NULL; \ - (agno) = (pag)->pag_agno + 1, \ - xfs_perag_rele(pag), \ - (pag) = xfs_perag_grab_tag((mp), (agno), (tag))) - static inline struct xfs_perag * xfs_perag_next_wrap( struct xfs_perag *pag, diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index cf629302d48e74..ac604640d36229 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -292,6 +292,64 @@ xfs_perag_clear_inode_tag( trace_xfs_perag_clear_inode_tag(pag, _RET_IP_); } +/* + * Search from @first to find the next perag with the given tag set. + */ +static struct xfs_perag * +xfs_perag_get_tag( + struct xfs_mount *mp, + xfs_agnumber_t first, + unsigned int tag) +{ + struct xfs_perag *pag; + int found; + + rcu_read_lock(); + found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, + (void **)&pag, first, 1, tag); + if (found <= 0) { + rcu_read_unlock(); + return NULL; + } + trace_xfs_perag_get_tag(pag, _RET_IP_); + atomic_inc(&pag->pag_ref); + rcu_read_unlock(); + return pag; +} + +/* + * Search from @first to find the next perag with the given tag set. + */ +static struct xfs_perag * +xfs_perag_grab_tag( + struct xfs_mount *mp, + xfs_agnumber_t first, + int tag) +{ + struct xfs_perag *pag; + int found; + + rcu_read_lock(); + found = radix_tree_gang_lookup_tag(&mp->m_perag_tree, + (void **)&pag, first, 1, tag); + if (found <= 0) { + rcu_read_unlock(); + return NULL; + } + trace_xfs_perag_grab_tag(pag, _RET_IP_); + if (!atomic_inc_not_zero(&pag->pag_active_ref)) + pag = NULL; + rcu_read_unlock(); + return pag; +} + +#define for_each_perag_tag(mp, agno, pag, tag) \ + for ((agno) = 0, (pag) = xfs_perag_grab_tag((mp), 0, (tag)); \ + (pag) != NULL; \ + (agno) = (pag)->pag_agno + 1, \ + xfs_perag_rele(pag), \ + (pag) = xfs_perag_grab_tag((mp), (agno), (tag))) + /* * When we recycle a reclaimable inode, we need to re-initialise the VFS inode * part of the structure. This is made more complex by the fact we store
The tagged perag helpers are only used in xfs_icache.c in the kernel code and not at all in xfsprogs. Move them to xfs_icache.c in preparation for switching to an xarray, for which I have no plan to implement the tagged lookup functions for userspace. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_ag.c | 51 ------------------------------------- fs/xfs/libxfs/xfs_ag.h | 11 -------- fs/xfs/xfs_icache.c | 58 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 62 deletions(-)