[RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Carlos Maiolino]

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.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;
-- 
2.26.2

Re: [RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Brian Foster]

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
> 

Re: [RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Darrick J. Wong]

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
> > 
> 

Re: [RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Christoph Hellwig]

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.

Re: [RFC PATCH] xfs: add inline helper to convert from data fork to xfs_attr_shortform

[Christoph Hellwig]

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 ?