| Message ID | 20200901095919.238598-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [RFC] xfs: add inline helper to convert from data fork to xfs_attr_shortform | expand |
On Tue, Sep 01, 2020 at 11:59:19AM +0200, Carlos Maiolino wrote: > Hi folks, while working on the attr structs cleanup, I've noticed there > are so many places where we do: > > (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; > > So, I thought it would be worth to add another inline function to do > this conversion and remove all these casts. > > To achieve this, it will be required to include xfs_inode.h header on > xfs_attr_sf.h, so it can access the xfs_inode definition. Also, if this > patch is an acceptable idea, it will make sense then to keep the > xfs_attr_sf_totsize() function also inside xfs_attr_sf.h (which has been > moved on my series to avoid the additional #include), so, I thought on > sending this RFC patch to get comments if it's a good idea or not, and, > if it is, I'll add this patch to my series before sending it over. > > I didn't focus on check if this patch is totally correct (only build > test), since my idea is to gather you guys opinions about having this > new inline function, so don't bother on reviewing the patch itself by > now, only the function name if you guys prefer some other name. > > Also, this patch is build on top of my clean up series (V2), not yet > sent to the list, so it won't apply anyway. > > Cheers. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/xfs/libxfs/xfs_attr.c | 4 ++-- > fs/xfs/libxfs/xfs_attr_leaf.c | 16 ++++++++-------- > fs/xfs/libxfs/xfs_attr_sf.h | 6 ++++++ > fs/xfs/xfs_attr_list.c | 2 +- > 4 files changed, 17 insertions(+), 11 deletions(-) > ... > diff --git a/fs/xfs/libxfs/xfs_attr_sf.h b/fs/xfs/libxfs/xfs_attr_sf.h > index 540ad3332a9c8..a51aed1dab6c1 100644 > --- a/fs/xfs/libxfs/xfs_attr_sf.h > +++ b/fs/xfs/libxfs/xfs_attr_sf.h > @@ -3,6 +3,8 @@ > * Copyright (c) 2000,2002,2005 Silicon Graphics, Inc. > * All Rights Reserved. > */ > + > +#include "xfs_inode.h" FWIW, I thought we tried to avoid including headers from other headers like this. I'm also wondering if it's an issue that we'd be including a a header that is external to libxfs from a libxfs header. Perhaps this could be simplified by passing the xfs_ifork pointer to the new helper rather than the xfs_inode and/or moving the helper to libxfs/xfs_inode_fork.h and putting a forward declaration of xfs_attr_shortform in there..? Brian > #ifndef __XFS_ATTR_SF_H__ > #define __XFS_ATTR_SF_H__ > > @@ -47,4 +49,8 @@ xfs_attr_sf_nextentry(struct xfs_attr_sf_entry *sfep) { > xfs_attr_sf_entsize(sfep)); > } > > +static inline struct xfs_attr_shortform * > +xfs_attr_ifork_to_sf(struct xfs_inode *ino) { > + return (struct xfs_attr_shortform *)ino->i_afp->if_u1.if_data; > +} > #endif /* __XFS_ATTR_SF_H__ */ > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > index 8f8837fe21cf0..7c0ebdeb43567 100644 > --- a/fs/xfs/xfs_attr_list.c > +++ b/fs/xfs/xfs_attr_list.c > @@ -61,7 +61,7 @@ xfs_attr_shortform_list( > int error = 0; > > ASSERT(dp->i_afp != NULL); > - sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; > + sf = xfs_attr_ifork_to_sf(dp); > ASSERT(sf != NULL); > if (!sf->hdr.count) > return 0; > -- > 2.26.2 >
On Tue, Sep 01, 2020 at 10:13:41AM -0400, Brian Foster wrote: > On Tue, Sep 01, 2020 at 11:59:19AM +0200, Carlos Maiolino wrote: > > Hi folks, while working on the attr structs cleanup, I've noticed there > > are so many places where we do: > > > > (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; > > > > So, I thought it would be worth to add another inline function to do > > this conversion and remove all these casts. > > > > To achieve this, it will be required to include xfs_inode.h header on > > xfs_attr_sf.h, so it can access the xfs_inode definition. Also, if this > > patch is an acceptable idea, it will make sense then to keep the > > xfs_attr_sf_totsize() function also inside xfs_attr_sf.h (which has been > > moved on my series to avoid the additional #include), so, I thought on > > sending this RFC patch to get comments if it's a good idea or not, and, > > if it is, I'll add this patch to my series before sending it over. > > > > I didn't focus on check if this patch is totally correct (only build > > test), since my idea is to gather you guys opinions about having this > > new inline function, so don't bother on reviewing the patch itself by > > now, only the function name if you guys prefer some other name. > > > > Also, this patch is build on top of my clean up series (V2), not yet > > sent to the list, so it won't apply anyway. > > > > Cheers. > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > fs/xfs/libxfs/xfs_attr.c | 4 ++-- > > fs/xfs/libxfs/xfs_attr_leaf.c | 16 ++++++++-------- > > fs/xfs/libxfs/xfs_attr_sf.h | 6 ++++++ > > fs/xfs/xfs_attr_list.c | 2 +- > > 4 files changed, 17 insertions(+), 11 deletions(-) > > > ... > > diff --git a/fs/xfs/libxfs/xfs_attr_sf.h b/fs/xfs/libxfs/xfs_attr_sf.h > > index 540ad3332a9c8..a51aed1dab6c1 100644 > > --- a/fs/xfs/libxfs/xfs_attr_sf.h > > +++ b/fs/xfs/libxfs/xfs_attr_sf.h > > @@ -3,6 +3,8 @@ > > * Copyright (c) 2000,2002,2005 Silicon Graphics, Inc. > > * All Rights Reserved. > > */ > > + > > +#include "xfs_inode.h" > > FWIW, I thought we tried to avoid including headers from other headers > like this. I'm also wondering if it's an issue that we'd be including a > a header that is external to libxfs from a libxfs header. Perhaps this > could be simplified by passing the xfs_ifork pointer to the new helper > rather than the xfs_inode and/or moving the helper to > libxfs/xfs_inode_fork.h and putting a forward declaration of > xfs_attr_shortform in there..? If you change if_data to a (void *), all the casts become unnecessary, right? --D > Brian > > > #ifndef __XFS_ATTR_SF_H__ > > #define __XFS_ATTR_SF_H__ > > > > @@ -47,4 +49,8 @@ xfs_attr_sf_nextentry(struct xfs_attr_sf_entry *sfep) { > > xfs_attr_sf_entsize(sfep)); > > } > > > > +static inline struct xfs_attr_shortform * > > +xfs_attr_ifork_to_sf(struct xfs_inode *ino) { > > + return (struct xfs_attr_shortform *)ino->i_afp->if_u1.if_data; > > +} > > #endif /* __XFS_ATTR_SF_H__ */ > > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > > index 8f8837fe21cf0..7c0ebdeb43567 100644 > > --- a/fs/xfs/xfs_attr_list.c > > +++ b/fs/xfs/xfs_attr_list.c > > @@ -61,7 +61,7 @@ xfs_attr_shortform_list( > > int error = 0; > > > > ASSERT(dp->i_afp != NULL); > > - sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; > > + sf = xfs_attr_ifork_to_sf(dp); > > ASSERT(sf != NULL); > > if (!sf->hdr.count) > > return 0; > > -- > > 2.26.2 > > >
On Tue, Sep 01, 2020 at 10:13:41AM -0400, Brian Foster wrote: > FWIW, I thought we tried to avoid including headers from other headers > like this. I'm also wondering if it's an issue that we'd be including a > a header that is external to libxfs from a libxfs header. Perhaps this > could be simplified by passing the xfs_ifork pointer to the new helper > rather than the xfs_inode and/or moving the helper to > libxfs/xfs_inode_fork.h and putting a forward declaration of > xfs_attr_shortform in there..? Or just turn it into a macro, which might be easiet? > > +static inline struct xfs_attr_shortform * > > +xfs_attr_ifork_to_sf(struct xfs_inode *ino) { > > + return (struct xfs_attr_shortform *)ino->i_afp->if_u1.if_data; > > +} I also find the name a little strange as it takes the inode maybe xfs_inode_to_attr_sf ?
On Tue, Sep 01, 2020 at 08:23:59AM -0700, Darrick J. Wong wrote: > > FWIW, I thought we tried to avoid including headers from other headers > > like this. I'm also wondering if it's an issue that we'd be including a > > a header that is external to libxfs from a libxfs header. Perhaps this > > could be simplified by passing the xfs_ifork pointer to the new helper > > rather than the xfs_inode and/or moving the helper to > > libxfs/xfs_inode_fork.h and putting a forward declaration of > > xfs_attr_shortform in there..? > > If you change if_data to a (void *), all the casts become unnecessary, > right? Yes, that is probably an even better idea.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index ff1fa2ed40ab9..1dccc8b9f31f6 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -525,8 +525,8 @@ xfs_attr_set( /* total space in use */ static inline int xfs_attr_sf_totsize(struct xfs_inode *dp) { - struct xfs_attr_shortform *sf = - (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; + struct xfs_attr_shortform *sf = xfs_attr_ifork_to_sf(dp); + return be16_to_cpu(sf->hdr.totsize); } diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index f64ab351b760c..ac92eba8745d6 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -681,7 +681,7 @@ xfs_attr_sf_findname( int end; int i; - sf = (struct xfs_attr_shortform *)args->dp->i_afp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(args->dp); sfe = &sf->list[0]; end = sf->hdr.count; for (i = 0; i < end; sfe = xfs_attr_sf_nextentry(sfe), @@ -728,14 +728,14 @@ xfs_attr_shortform_add( ifp = dp->i_afp; ASSERT(ifp->if_flags & XFS_IFINLINE); - sf = (struct xfs_attr_shortform *)ifp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(dp); if (xfs_attr_sf_findname(args, &sfe, NULL) == -EEXIST) ASSERT(0); offset = (char *)sfe - (char *)sf; size = xfs_attr_sf_entsize_byname(args->namelen, args->valuelen); xfs_idata_realloc(dp, size, XFS_ATTR_FORK); - sf = (struct xfs_attr_shortform *)ifp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(dp); sfe = (struct xfs_attr_sf_entry *)((char *)sf + offset); sfe->namelen = args->namelen; @@ -787,7 +787,7 @@ xfs_attr_shortform_remove( dp = args->dp; mp = dp->i_mount; - sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(dp); error = xfs_attr_sf_findname(args, &sfe, &base); if (error != -EEXIST) @@ -846,7 +846,7 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args) ifp = args->dp->i_afp; ASSERT(ifp->if_flags & XFS_IFINLINE); - sf = (struct xfs_attr_shortform *)ifp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(args->dp); sfe = &sf->list[0]; for (i = 0; i < sf->hdr.count; sfe = xfs_attr_sf_nextentry(sfe), i++) { @@ -873,7 +873,7 @@ xfs_attr_shortform_getvalue( int i; ASSERT(args->dp->i_afp->if_flags == XFS_IFINLINE); - sf = (struct xfs_attr_shortform *)args->dp->i_afp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(args->dp); sfe = &sf->list[0]; for (i = 0; i < sf->hdr.count; sfe = xfs_attr_sf_nextentry(sfe), i++) { @@ -908,7 +908,7 @@ xfs_attr_shortform_to_leaf( dp = args->dp; ifp = dp->i_afp; - sf = (struct xfs_attr_shortform *)ifp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(dp); size = be16_to_cpu(sf->hdr.totsize); tmpbuffer = kmem_alloc(size, 0); ASSERT(tmpbuffer != NULL); @@ -1018,7 +1018,7 @@ xfs_attr_shortform_verify( ASSERT(ip->i_afp->if_format == XFS_DINODE_FMT_LOCAL); ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK); - sfp = (struct xfs_attr_shortform *)ifp->if_u1.if_data; + sfp = xfs_attr_ifork_to_sf(ip); size = ifp->if_bytes; /* diff --git a/fs/xfs/libxfs/xfs_attr_sf.h b/fs/xfs/libxfs/xfs_attr_sf.h index 540ad3332a9c8..a51aed1dab6c1 100644 --- a/fs/xfs/libxfs/xfs_attr_sf.h +++ b/fs/xfs/libxfs/xfs_attr_sf.h @@ -3,6 +3,8 @@ * Copyright (c) 2000,2002,2005 Silicon Graphics, Inc. * All Rights Reserved. */ + +#include "xfs_inode.h" #ifndef __XFS_ATTR_SF_H__ #define __XFS_ATTR_SF_H__ @@ -47,4 +49,8 @@ xfs_attr_sf_nextentry(struct xfs_attr_sf_entry *sfep) { xfs_attr_sf_entsize(sfep)); } +static inline struct xfs_attr_shortform * +xfs_attr_ifork_to_sf(struct xfs_inode *ino) { + return (struct xfs_attr_shortform *)ino->i_afp->if_u1.if_data; +} #endif /* __XFS_ATTR_SF_H__ */ diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c index 8f8837fe21cf0..7c0ebdeb43567 100644 --- a/fs/xfs/xfs_attr_list.c +++ b/fs/xfs/xfs_attr_list.c @@ -61,7 +61,7 @@ xfs_attr_shortform_list( int error = 0; ASSERT(dp->i_afp != NULL); - sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; + sf = xfs_attr_ifork_to_sf(dp); ASSERT(sf != NULL); if (!sf->hdr.count) return 0;
Hi folks, while working on the attr structs cleanup, I've noticed there are so many places where we do: (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data; So, I thought it would be worth to add another inline function to do this conversion and remove all these casts. To achieve this, it will be required to include xfs_inode.h header on xfs_attr_sf.h, so it can access the xfs_inode definition. Also, if this patch is an acceptable idea, it will make sense then to keep the xfs_attr_sf_totsize() function also inside xfs_attr_sf.h (which has been moved on my series to avoid the additional #include), so, I thought on sending this RFC patch to get comments if it's a good idea or not, and, if it is, I'll add this patch to my series before sending it over. I didn't focus on check if this patch is totally correct (only build test), since my idea is to gather you guys opinions about having this new inline function, so don't bother on reviewing the patch itself by now, only the function name if you guys prefer some other name. Also, this patch is build on top of my clean up series (V2), not yet sent to the list, so it won't apply anyway. Cheers. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/xfs/libxfs/xfs_attr.c | 4 ++-- fs/xfs/libxfs/xfs_attr_leaf.c | 16 ++++++++-------- fs/xfs/libxfs/xfs_attr_sf.h | 6 ++++++ fs/xfs/xfs_attr_list.c | 2 +- 4 files changed, 17 insertions(+), 11 deletions(-)