diff mbox series

[06/22] xfs: refactor kernel-specific parts of xfs_ialloc

Message ID 154630917828.18437.11659667761358440410.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: hoist inode operations to libxfs | expand

Commit Message

Darrick J. Wong Jan. 1, 2019, 2:19 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Move the kernel-specific parts of xfs_ialloc into a separate function in
preparation for hoisting xfs_ialloc to libxfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_inode.c |   45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

Comments

Christoph Hellwig Jan. 17, 2019, 2:22 p.m. UTC | #1 
On Mon, Dec 31, 2018 at 06:19:38PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the kernel-specific parts of xfs_ialloc into a separate function in
> preparation for hoisting xfs_ialloc to libxfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c |   45 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 403eb8fa2f1f..b635d43caeed 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -621,6 +621,32 @@ xfs_lookup(
>  	return error;
>  }
>  
> +/* Return an exclusive ILOCK'd in-core inode. */
> +static int
> +xfs_ialloc_iget(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	*tp,
> +	xfs_ino_t		ino,
> +	struct xfs_inode	**ipp)
> +{
> +	return xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, ipp);
> +}

This is just too ugly and pointless.  Can you explain (or even better
show code) why we really need this for xfsprogs?

> +	xfs_ialloc_platform_init(tp, args, ip);

And that platform naming also is rather horrible.
Dave Chinner Jan. 17, 2019, 10:26 p.m. UTC | #2 
On Mon, Dec 31, 2018 at 06:19:38PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the kernel-specific parts of xfs_ialloc into a separate function in
> preparation for hoisting xfs_ialloc to libxfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c |   45 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 403eb8fa2f1f..b635d43caeed 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -621,6 +621,32 @@ xfs_lookup(
>  	return error;
>  }
>  
> +/* Return an exclusive ILOCK'd in-core inode. */
> +static int
> +xfs_ialloc_iget(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	*tp,
> +	xfs_ino_t		ino,
> +	struct xfs_inode	**ipp)
> +{
> +	return xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, ipp);
> +}
> +
> +/* Propagate platform-specific inode properties into the new child. */
> +static void
> +xfs_ialloc_platform_init(
> +	struct xfs_trans		*tp,
> +	const struct xfs_ialloc_args	*args,
> +	struct xfs_inode		*ip)
> +{
> +	struct timespec64		tv;
> +
> +	tv = current_time(VFS_I(ip));
> +	VFS_I(ip)->i_mtime = tv;
> +	VFS_I(ip)->i_atime = tv;
> +	VFS_I(ip)->i_ctime = tv;

Doesn't set ip->i_d.di_crtime, so ....

> @@ -755,10 +781,11 @@ xfs_ialloc(
>  		inode_set_iversion(inode, 1);
>  		ip->i_d.di_flags2 = 0;
>  		ip->i_d.di_cowextsize = 0;
> -		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> -		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> +		ip->i_d.di_crtime.t_sec = 0;
> +		ip->i_d.di_crtime.t_nsec = 0;
>  	}
>  
> +	xfs_ialloc_platform_init(tp, args, ip);

This breaks create time functionality.

Don't we have a statx() test in fstests that checks that the
create time of inodes is set correctly?

Cheers,

Dave.
Darrick J. Wong Jan. 17, 2019, 11:02 p.m. UTC | #3 
On Fri, Jan 18, 2019 at 09:26:14AM +1100, Dave Chinner wrote:
> On Mon, Dec 31, 2018 at 06:19:38PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move the kernel-specific parts of xfs_ialloc into a separate function in
> > preparation for hoisting xfs_ialloc to libxfs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c |   45 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 36 insertions(+), 9 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 403eb8fa2f1f..b635d43caeed 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -621,6 +621,32 @@ xfs_lookup(
> >  	return error;
> >  }
> >  
> > +/* Return an exclusive ILOCK'd in-core inode. */
> > +static int
> > +xfs_ialloc_iget(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	xfs_ino_t		ino,
> > +	struct xfs_inode	**ipp)
> > +{
> > +	return xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, ipp);
> > +}
> > +
> > +/* Propagate platform-specific inode properties into the new child. */
> > +static void
> > +xfs_ialloc_platform_init(
> > +	struct xfs_trans		*tp,
> > +	const struct xfs_ialloc_args	*args,
> > +	struct xfs_inode		*ip)
> > +{
> > +	struct timespec64		tv;
> > +
> > +	tv = current_time(VFS_I(ip));
> > +	VFS_I(ip)->i_mtime = tv;
> > +	VFS_I(ip)->i_atime = tv;
> > +	VFS_I(ip)->i_ctime = tv;
> 
> Doesn't set ip->i_d.di_crtime, so ....
> 
> > @@ -755,10 +781,11 @@ xfs_ialloc(
> >  		inode_set_iversion(inode, 1);
> >  		ip->i_d.di_flags2 = 0;
> >  		ip->i_d.di_cowextsize = 0;
> > -		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> > -		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> > +		ip->i_d.di_crtime.t_sec = 0;
> > +		ip->i_d.di_crtime.t_nsec = 0;
> >  	}
> >  
> > +	xfs_ialloc_platform_init(tp, args, ip);
> 
> This breaks create time functionality.

Yikes!

> Don't we have a statx() test in fstests that checks that the
> create time of inodes is set correctly?

Clearly not... <frown>

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Darrick J. Wong Jan. 17, 2019, 11:28 p.m. UTC | #4 
On Thu, Jan 17, 2019 at 06:22:21AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 31, 2018 at 06:19:38PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move the kernel-specific parts of xfs_ialloc into a separate function in
> > preparation for hoisting xfs_ialloc to libxfs.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c |   45 ++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 36 insertions(+), 9 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 403eb8fa2f1f..b635d43caeed 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -621,6 +621,32 @@ xfs_lookup(
> >  	return error;
> >  }
> >  
> > +/* Return an exclusive ILOCK'd in-core inode. */
> > +static int
> > +xfs_ialloc_iget(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	xfs_ino_t		ino,
> > +	struct xfs_inode	**ipp)
> > +{
> > +	return xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, ipp);
> > +}
> 
> This is just too ugly and pointless.  Can you explain (or even better
> show code) why we really need this for xfsprogs?

To avoid having a useless lock_flags argument to xfs_iget in userspace?
It wasn't so long ago that Eric removed it, but I don't mind adding it
back to avoid this kind of ugliness.

> > +	xfs_ialloc_platform_init(tp, args, ip);
> 
> And that platform naming also is rather horrible.

Hm.  Looking at it now, both platform_init() functions basically just
set ctime/mtime/atime/crtime, which means that I really just need to add
a XFS_ICHGTIME_CREATE flag to xfs_trans_ichgtime and replace all the
platform_init stuff with that.

The mkfs protofile platform_init also has some code to set other
attributes from a struct fsxattr, but I think I can just set them once
the inode allocation function returns to the protofile code.

--D
Christoph Hellwig Jan. 18, 2019, 7:17 a.m. UTC | #5 
On Thu, Jan 17, 2019 at 03:28:08PM -0800, Darrick J. Wong wrote:
> > This is just too ugly and pointless.  Can you explain (or even better
> > show code) why we really need this for xfsprogs?
> 
> To avoid having a useless lock_flags argument to xfs_iget in userspace?
> It wasn't so long ago that Eric removed it, but I don't mind adding it
> back to avoid this kind of ugliness.

I'd rather have prototypes match in userspace then working around
mismatches in the kernel.

> The mkfs protofile platform_init also has some code to set other
> attributes from a struct fsxattr, but I think I can just set them once
> the inode allocation function returns to the protofile code.

And we are killing that anyway, right?
Darrick J. Wong Jan. 24, 2019, 2:04 a.m. UTC | #6 
On Thu, Jan 17, 2019 at 11:17:08PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 17, 2019 at 03:28:08PM -0800, Darrick J. Wong wrote:
> > > This is just too ugly and pointless.  Can you explain (or even better
> > > show code) why we really need this for xfsprogs?
> > 
> > To avoid having a useless lock_flags argument to xfs_iget in userspace?
> > It wasn't so long ago that Eric removed it, but I don't mind adding it
> > back to avoid this kind of ugliness.
> 
> I'd rather have prototypes match in userspace then working around
> mismatches in the kernel.

The other thing about the userspace iget is that you can pass in a
custom set of inode fork verifiers.  xfs_repair uses this to deal with
broken '..' pointers by leaving them unfixed until phase 6, at which
point it knows enough to reset the pointer, but then wants to be able to
iget the directory to fix it.

We could work around that with a libxfs_iget_fork_ops variant so that
xfs_iget remains the same between kernel and userspace.  How about that?

> > The mkfs protofile platform_init also has some code to set other
> > attributes from a struct fsxattr, but I think I can just set them once
> > the inode allocation function returns to the protofile code.
> 
> And we are killing that anyway, right?

Yeah.  I think?  Eric? :)

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 403eb8fa2f1f..b635d43caeed 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -621,6 +621,32 @@  xfs_lookup(
 	return error;
 }
 
+/* Return an exclusive ILOCK'd in-core inode. */
+static int
+xfs_ialloc_iget(
+	struct xfs_mount	*mp,
+	struct xfs_trans	*tp,
+	xfs_ino_t		ino,
+	struct xfs_inode	**ipp)
+{
+	return xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, ipp);
+}
+
+/* Propagate platform-specific inode properties into the new child. */
+static void
+xfs_ialloc_platform_init(
+	struct xfs_trans		*tp,
+	const struct xfs_ialloc_args	*args,
+	struct xfs_inode		*ip)
+{
+	struct timespec64		tv;
+
+	tv = current_time(VFS_I(ip));
+	VFS_I(ip)->i_mtime = tv;
+	VFS_I(ip)->i_atime = tv;
+	VFS_I(ip)->i_ctime = tv;
+}
+
 /*
  * Allocate an inode on disk and return a copy of its in-core version.
  * The in-core inode is locked exclusively.  Set mode, nlink, and rdev
@@ -666,7 +692,6 @@  xfs_ialloc(
 	xfs_ino_t			ino;
 	uint				flags;
 	int				error;
-	struct timespec64		tv;
 
 	/*
 	 * Call the space management code to pick
@@ -699,8 +724,7 @@  xfs_ialloc(
 	 * This is because we're setting fields here we need
 	 * to prevent others from looking at until we're done.
 	 */
-	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
-			 XFS_ILOCK_EXCL, &ip);
+	error = xfs_ialloc_iget(mp, tp, ino, &ip);
 	if (error)
 		return error;
 	ASSERT(ip != NULL);
@@ -741,10 +765,12 @@  xfs_ialloc(
 	ip->i_d.di_nextents = 0;
 	ASSERT(ip->i_d.di_nblocks == 0);
 
-	tv = current_time(inode);
-	inode->i_mtime = tv;
-	inode->i_atime = tv;
-	inode->i_ctime = tv;
+	inode->i_mtime.tv_sec = 0;
+	inode->i_mtime.tv_nsec = 0;
+	inode->i_atime.tv_sec = 0;
+	inode->i_atime.tv_nsec = 0;
+	inode->i_ctime.tv_sec = 0;
+	inode->i_ctime.tv_nsec = 0;
 
 	ip->i_d.di_extsize = 0;
 	ip->i_d.di_dmevmask = 0;
@@ -755,10 +781,11 @@  xfs_ialloc(
 		inode_set_iversion(inode, 1);
 		ip->i_d.di_flags2 = 0;
 		ip->i_d.di_cowextsize = 0;
-		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
-		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
+		ip->i_d.di_crtime.t_sec = 0;
+		ip->i_d.di_crtime.t_nsec = 0;
 	}
 
+	xfs_ialloc_platform_init(tp, args, ip);
 
 	flags = XFS_ILOG_CORE;
 	switch (args->mode & S_IFMT) {