[PATCH] xfs: Add a helper to retrieve xfs_inode from the address_space

[PATCH] xfs: Add a helper to retrieve xfs_inode from the address_space

[Carlos Maiolino]

Reduce some code and local variable allocation on file operations by
using a new helper.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---

Hi, I though this could be useful instead of keeping allocating file,
address_space and inode structs in several places, just to retrieve the
xfs_inode, what you guys think?

XFS_FTOINO() isn't really a good name, that's the first one which came to my
mind, if this is useful at all I can change the name to something else, like
XFS_AS_TO_INO() maybe, or something else?!

Although I think some vfs_wide helper like file_inode() would be more useful,
but, well, it's just an idea that came to my mind.

Cheers

 fs/xfs/xfs_file.c  | 37 ++++++++++++++-----------------------
 fs/xfs/xfs_inode.h |  5 +++++
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 299aee4b7b0b..b57e39a78ff6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -95,7 +95,7 @@ xfs_dir_fsync(
 	loff_t			end,
 	int			datasync)
 {
-	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
+	struct xfs_inode	*ip = XFS_FTOINO(file);
 	struct xfs_mount	*mp = ip->i_mount;
 	xfs_lsn_t		lsn = 0;
 
@@ -118,8 +118,7 @@ xfs_file_fsync(
 	loff_t			end,
 	int			datasync)
 {
-	struct inode		*inode = file->f_mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(file);
 	struct xfs_mount	*mp = ip->i_mount;
 	int			error = 0;
 	int			log_flushed = 0;
@@ -215,7 +214,7 @@ xfs_file_dax_read(
 	struct kiocb		*iocb,
 	struct iov_iter		*to)
 {
-	struct xfs_inode	*ip = XFS_I(iocb->ki_filp->f_mapping->host);
+	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
 	size_t			count = iov_iter_count(to);
 	ssize_t			ret = 0;
 
@@ -300,8 +299,8 @@ xfs_file_aio_write_checks(
 	int			*iolock)
 {
 	struct file		*file = iocb->ki_filp;
-	struct inode		*inode = file->f_mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(file);
+	struct inode		*inode = VFS_I(ip);
 	ssize_t			error = 0;
 	size_t			count = iov_iter_count(from);
 	bool			drained_dio = false;
@@ -364,7 +363,7 @@ xfs_file_aio_write_checks(
 			drained_dio = true;
 			goto restart;
 		}
-	
+
 		trace_xfs_zero_eof(ip, isize, iocb->ki_pos - isize);
 		error = iomap_zero_range(inode, isize, iocb->ki_pos - isize,
 				NULL, &xfs_iomap_ops);
@@ -482,10 +481,8 @@ xfs_file_dio_aio_write(
 	struct kiocb		*iocb,
 	struct iov_iter		*from)
 {
-	struct file		*file = iocb->ki_filp;
-	struct address_space	*mapping = file->f_mapping;
-	struct inode		*inode = mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
+	struct inode		*inode = VFS_I(ip);
 	struct xfs_mount	*mp = ip->i_mount;
 	ssize_t			ret = 0;
 	int			unaligned_io = 0;
@@ -570,8 +567,8 @@ xfs_file_dax_write(
 	struct kiocb		*iocb,
 	struct iov_iter		*from)
 {
-	struct inode		*inode = iocb->ki_filp->f_mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
+	struct inode		*inode = VFS_I(ip);
 	int			iolock = XFS_IOLOCK_EXCL;
 	ssize_t			ret, error = 0;
 	size_t			count;
@@ -607,10 +604,7 @@ xfs_file_buffered_aio_write(
 	struct kiocb		*iocb,
 	struct iov_iter		*from)
 {
-	struct file		*file = iocb->ki_filp;
-	struct address_space	*mapping = file->f_mapping;
-	struct inode		*inode = mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
 	ssize_t			ret;
 	int			enospc = 0;
 	int			iolock;
@@ -627,7 +621,7 @@ xfs_file_buffered_aio_write(
 		goto out;
 
 	/* We can write back this queue in page reclaim */
-	current->backing_dev_info = inode_to_bdi(inode);
+	current->backing_dev_info = inode_to_bdi(VFS_I(ip));
 
 	trace_xfs_file_buffered_write(ip, iov_iter_count(from), iocb->ki_pos);
 	ret = iomap_file_buffered_write(iocb, from, &xfs_iomap_ops);
@@ -677,10 +671,7 @@ xfs_file_write_iter(
 	struct kiocb		*iocb,
 	struct iov_iter		*from)
 {
-	struct file		*file = iocb->ki_filp;
-	struct address_space	*mapping = file->f_mapping;
-	struct inode		*inode = mapping->host;
-	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
 	ssize_t			ret;
 	size_t			ocount = iov_iter_count(from);
 
@@ -692,7 +683,7 @@ xfs_file_write_iter(
 	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
 		return -EIO;
 
-	if (IS_DAX(inode))
+	if (IS_DAX(VFS_I(ip)))
 		ret = xfs_file_dax_write(iocb, from);
 	else if (iocb->ki_flags & IOCB_DIRECT) {
 		/*
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 1eebc53df7d7..dd359644de22 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -84,6 +84,11 @@ static inline struct inode *VFS_I(struct xfs_inode *ip)
 	return &ip->i_vnode;
 }
 
+/* convert from file to xfs inode */
+static inline struct xfs_inode *XFS_FTOINO(struct file *filp)
+{
+	return XFS_I(filp->f_mapping->host);
+}
 /*
  * For regular files we only update the on-disk filesize when actually
  * writing data back to disk.  Until then only the copy in the VFS inode
-- 
2.14.3

Re: [PATCH] xfs: Add a helper to retrieve xfs_inode from the address_space

[Darrick J. Wong]

On Thu, Apr 19, 2018 at 04:35:11PM +0200, Carlos Maiolino wrote:
> Reduce some code and local variable allocation on file operations by
> using a new helper.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> Hi, I though this could be useful instead of keeping allocating file,
> address_space and inode structs in several places, just to retrieve the
> xfs_inode, what you guys think?
> 
> XFS_FTOINO() isn't really a good name, that's the first one which came to my
> mind, if this is useful at all I can change the name to something else, like
> XFS_AS_TO_INO() maybe, or something else?!
> 
> Although I think some vfs_wide helper like file_inode() would be more useful,
> but, well, it's just an idea that came to my mind.

There's already a file_inode() defined in include/linux/fs.h.

Though now I wonder if there's supposed to be a difference between
file->f_inode and file->f_mapping->host?  It doesn't look like it, but
An Engineer Put It There(tm). :)

(I dunno, some of the other filesystems do tricky things with metadata
inodes...)

--D

> 
> Cheers
> 
>  fs/xfs/xfs_file.c  | 37 ++++++++++++++-----------------------
>  fs/xfs/xfs_inode.h |  5 +++++
>  2 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 299aee4b7b0b..b57e39a78ff6 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -95,7 +95,7 @@ xfs_dir_fsync(
>  	loff_t			end,
>  	int			datasync)
>  {
> -	struct xfs_inode	*ip = XFS_I(file->f_mapping->host);
> +	struct xfs_inode	*ip = XFS_FTOINO(file);
>  	struct xfs_mount	*mp = ip->i_mount;
>  	xfs_lsn_t		lsn = 0;
>  
> @@ -118,8 +118,7 @@ xfs_file_fsync(
>  	loff_t			end,
>  	int			datasync)
>  {
> -	struct inode		*inode = file->f_mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(file);
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			error = 0;
>  	int			log_flushed = 0;
> @@ -215,7 +214,7 @@ xfs_file_dax_read(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*to)
>  {
> -	struct xfs_inode	*ip = XFS_I(iocb->ki_filp->f_mapping->host);
> +	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
>  	size_t			count = iov_iter_count(to);
>  	ssize_t			ret = 0;
>  
> @@ -300,8 +299,8 @@ xfs_file_aio_write_checks(
>  	int			*iolock)
>  {
>  	struct file		*file = iocb->ki_filp;
> -	struct inode		*inode = file->f_mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(file);
> +	struct inode		*inode = VFS_I(ip);
>  	ssize_t			error = 0;
>  	size_t			count = iov_iter_count(from);
>  	bool			drained_dio = false;
> @@ -364,7 +363,7 @@ xfs_file_aio_write_checks(
>  			drained_dio = true;
>  			goto restart;
>  		}
> -	
> +
>  		trace_xfs_zero_eof(ip, isize, iocb->ki_pos - isize);
>  		error = iomap_zero_range(inode, isize, iocb->ki_pos - isize,
>  				NULL, &xfs_iomap_ops);
> @@ -482,10 +481,8 @@ xfs_file_dio_aio_write(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*from)
>  {
> -	struct file		*file = iocb->ki_filp;
> -	struct address_space	*mapping = file->f_mapping;
> -	struct inode		*inode = mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
> +	struct inode		*inode = VFS_I(ip);
>  	struct xfs_mount	*mp = ip->i_mount;
>  	ssize_t			ret = 0;
>  	int			unaligned_io = 0;
> @@ -570,8 +567,8 @@ xfs_file_dax_write(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*from)
>  {
> -	struct inode		*inode = iocb->ki_filp->f_mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
> +	struct inode		*inode = VFS_I(ip);
>  	int			iolock = XFS_IOLOCK_EXCL;
>  	ssize_t			ret, error = 0;
>  	size_t			count;
> @@ -607,10 +604,7 @@ xfs_file_buffered_aio_write(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*from)
>  {
> -	struct file		*file = iocb->ki_filp;
> -	struct address_space	*mapping = file->f_mapping;
> -	struct inode		*inode = mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
>  	ssize_t			ret;
>  	int			enospc = 0;
>  	int			iolock;
> @@ -627,7 +621,7 @@ xfs_file_buffered_aio_write(
>  		goto out;
>  
>  	/* We can write back this queue in page reclaim */
> -	current->backing_dev_info = inode_to_bdi(inode);
> +	current->backing_dev_info = inode_to_bdi(VFS_I(ip));
>  
>  	trace_xfs_file_buffered_write(ip, iov_iter_count(from), iocb->ki_pos);
>  	ret = iomap_file_buffered_write(iocb, from, &xfs_iomap_ops);
> @@ -677,10 +671,7 @@ xfs_file_write_iter(
>  	struct kiocb		*iocb,
>  	struct iov_iter		*from)
>  {
> -	struct file		*file = iocb->ki_filp;
> -	struct address_space	*mapping = file->f_mapping;
> -	struct inode		*inode = mapping->host;
> -	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_inode	*ip = XFS_FTOINO(iocb->ki_filp);
>  	ssize_t			ret;
>  	size_t			ocount = iov_iter_count(from);
>  
> @@ -692,7 +683,7 @@ xfs_file_write_iter(
>  	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
>  		return -EIO;
>  
> -	if (IS_DAX(inode))
> +	if (IS_DAX(VFS_I(ip)))
>  		ret = xfs_file_dax_write(iocb, from);
>  	else if (iocb->ki_flags & IOCB_DIRECT) {
>  		/*
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 1eebc53df7d7..dd359644de22 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -84,6 +84,11 @@ static inline struct inode *VFS_I(struct xfs_inode *ip)
>  	return &ip->i_vnode;
>  }
>  
> +/* convert from file to xfs inode */
> +static inline struct xfs_inode *XFS_FTOINO(struct file *filp)
> +{
> +	return XFS_I(filp->f_mapping->host);
> +}
>  /*
>   * For regular files we only update the on-disk filesize when actually
>   * writing data back to disk.  Until then only the copy in the VFS inode
> -- 
> 2.14.3
> 
> --
> 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

Re: [PATCH] xfs: Add a helper to retrieve xfs_inode from the address_space

[Dave Chinner]

On Thu, Apr 19, 2018 at 04:35:11PM +0200, Carlos Maiolino wrote:
> Reduce some code and local variable allocation on file operations by
> using a new helper.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> 
> Hi, I though this could be useful instead of keeping allocating file,
> address_space and inode structs in several places, just to retrieve the
> xfs_inode, what you guys think?
> 
> XFS_FTOINO() isn't really a good name, that's the first one which came to my
> mind, if this is useful at all I can change the name to something else, like
> XFS_AS_TO_INO() maybe, or something else?!

I'd prefer that we don't add more "vfs to xfs structure" macros like
this. It doesn't make the code any more readable or correct,
especially for non-XFS developers.

> Although I think some vfs_wide helper like file_inode() would be more useful,
> but, well, it's just an idea that came to my mind.

XFS_I(file_inode(file)) would be my preferred solution here, as it
uses the correct VFS accessor function to get the VFS inode from
the struct file and it's obvious what it does to anyone familiar
with typical VFS and filesystem coding conventions.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: Add a helper to retrieve xfs_inode from the address_space

[Carlos Maiolino]

On Fri, Apr 20, 2018 at 08:50:02AM +1000, Dave Chinner wrote:
> On Thu, Apr 19, 2018 at 04:35:11PM +0200, Carlos Maiolino wrote:
> > Reduce some code and local variable allocation on file operations by
> > using a new helper.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> > 
> > Hi, I though this could be useful instead of keeping allocating file,
> > address_space and inode structs in several places, just to retrieve the
> > xfs_inode, what you guys think?
> > 
> > XFS_FTOINO() isn't really a good name, that's the first one which came to my
> > mind, if this is useful at all I can change the name to something else, like
> > XFS_AS_TO_INO() maybe, or something else?!
> 
> I'd prefer that we don't add more "vfs to xfs structure" macros like
> this. It doesn't make the code any more readable or correct,
> especially for non-XFS developers.
> 

Ok.

> > Although I think some vfs_wide helper like file_inode() would be more useful,
> > but, well, it's just an idea that came to my mind.
> 
> XFS_I(file_inode(file)) would be my preferred solution here, as it
> uses the correct VFS accessor function to get the VFS inode from
> the struct file and it's obvious what it does to anyone familiar
> with typical VFS and filesystem coding conventions.
> 

I think keep nesting function calls to reduce a few lines of code if harder to
read indeed, and I would stay with the current way and pretend I've never
submitted this patch :)

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

-- 
Carlos