diff mbox series

[v3,15/26] xfs: add parent attributes to link

Message ID 20220922054458.40826-16-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show
Series Parent Pointers | expand

Commit Message

Allison Henderson Sept. 22, 2022, 5:44 a.m. UTC 
From: Allison Henderson <allison.henderson@oracle.com>

This patch modifies xfs_link to add a parent pointer to the inode.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/xfs_inode.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

Comments

Darrick J. Wong Sept. 23, 2022, 8:31 p.m. UTC | #1 
On Wed, Sep 21, 2022 at 10:44:47PM -0700, allison.henderson@oracle.com wrote:
> From: Allison Henderson <allison.henderson@oracle.com>
> 
> This patch modifies xfs_link to add a parent pointer to the inode.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_inode.c | 44 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 181d6417412e..af3f5edb7319 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1228,14 +1228,16 @@ xfs_create_tmpfile(
>  
>  int
>  xfs_link(
> -	xfs_inode_t		*tdp,
> -	xfs_inode_t		*sip,
> +	struct xfs_inode	*tdp,
> +	struct xfs_inode	*sip,
>  	struct xfs_name		*target_name)
>  {
> -	xfs_mount_t		*mp = tdp->i_mount;
> -	xfs_trans_t		*tp;
> +	struct xfs_mount	*mp = tdp->i_mount;
> +	struct xfs_trans	*tp;
>  	int			error, nospace_error = 0;
>  	int			resblks;
> +	xfs_dir2_dataptr_t	diroffset;
> +	struct xfs_parent_defer	*parent = NULL;
>  
>  	trace_xfs_link(tdp, target_name);
>  
> @@ -1252,11 +1254,17 @@ xfs_link(
>  	if (error)
>  		goto std_return;
>  
> +	if (xfs_has_parent(mp)) {
> +		error = xfs_parent_init(mp, &parent);
> +		if (error)
> +			goto std_return;
> +	}
> +
>  	resblks = XFS_LINK_SPACE_RES(mp, target_name->len);

Forwarding on from the v2 series --

This patch ought to be modifying XFS_LINK_SPACE_RES so that each link()
update reserves enough space to handle an expansion in the tdp directory
(as it does now) *and* an expansion in the xattr structure of the sip
child file.  This is how we avoid dipping into the free space reserve
pool midway through a transaction, and avoid shutdowns when space is
tight.

tr_res == space we reserve in the *log* to record updates.

XFS_LINK_SPACE_RES == block we reserve from the filesystem free space to
handle expansions of metadata structures.

At this point in this version of the patchset, you've increased the log
space reservations in anticipation of logging more information per
transaction.  However, you've not increased the free space reservations
to handle potential node splitting in the ondisk xattr btree.

(The rest of the patchset looks ok.)

--D

>  	error = xfs_trans_alloc_dir(tdp, &M_RES(mp)->tr_link, sip, &resblks,
>  			&tp, &nospace_error);
>  	if (error)
> -		goto std_return;
> +		goto drop_incompat;
>  
>  	/*
>  	 * If we are using project inheritance, we only allow hard link
> @@ -1289,14 +1297,27 @@ xfs_link(
>  	}
>  
>  	error = xfs_dir_createname(tp, tdp, target_name, sip->i_ino,
> -				   resblks, NULL);
> +				   resblks, &diroffset);
>  	if (error)
> -		goto error_return;
> +		goto out_defer_cancel;
>  	xfs_trans_ichgtime(tp, tdp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>  	xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
>  
>  	xfs_bumplink(tp, sip);
>  
> +	/*
> +	 * If we have parent pointers, we now need to add the parent record to
> +	 * the attribute fork of the inode. If this is the initial parent
> +	 * attribute, we need to create it correctly, otherwise we can just add
> +	 * the parent to the inode.
> +	 */
> +	if (parent) {
> +		error = xfs_parent_defer_add(tp, parent, tdp, target_name,
> +					     diroffset, sip);
> +		if (error)
> +			goto out_defer_cancel;
> +	}
> +
>  	/*
>  	 * If this is a synchronous mount, make sure that the
>  	 * link transaction goes to disk before returning to
> @@ -1310,11 +1331,16 @@ xfs_link(
>  	xfs_iunlock(sip, XFS_ILOCK_EXCL);
>  	return error;
>  
> - error_return:
> +out_defer_cancel:
> +	xfs_defer_cancel(tp);
> +error_return:
>  	xfs_trans_cancel(tp);
>  	xfs_iunlock(tdp, XFS_ILOCK_EXCL);
>  	xfs_iunlock(sip, XFS_ILOCK_EXCL);
> - std_return:
> +drop_incompat:
> +	if (parent)
> +		xfs_parent_cancel(mp, parent);
> +std_return:
>  	if (error == -ENOSPC && nospace_error)
>  		error = nospace_error;
>  	return error;
> -- 
> 2.25.1
>
Allison Henderson Sept. 26, 2022, 9:49 p.m. UTC | #2 
On Fri, 2022-09-23 at 13:31 -0700, Darrick J. Wong wrote:
> On Wed, Sep 21, 2022 at 10:44:47PM -0700,
> allison.henderson@oracle.com wrote:
> > From: Allison Henderson <allison.henderson@oracle.com>
> > 
> > This patch modifies xfs_link to add a parent pointer to the inode.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c | 44 +++++++++++++++++++++++++++++++++++-------
> > --
> >  1 file changed, 35 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 181d6417412e..af3f5edb7319 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1228,14 +1228,16 @@ xfs_create_tmpfile(
> >  
> >  int
> >  xfs_link(
> > -       xfs_inode_t             *tdp,
> > -       xfs_inode_t             *sip,
> > +       struct xfs_inode        *tdp,
> > +       struct xfs_inode        *sip,
> >         struct xfs_name         *target_name)
> >  {
> > -       xfs_mount_t             *mp = tdp->i_mount;
> > -       xfs_trans_t             *tp;
> > +       struct xfs_mount        *mp = tdp->i_mount;
> > +       struct xfs_trans        *tp;
> >         int                     error, nospace_error = 0;
> >         int                     resblks;
> > +       xfs_dir2_dataptr_t      diroffset;
> > +       struct xfs_parent_defer *parent = NULL;
> >  
> >         trace_xfs_link(tdp, target_name);
> >  
> > @@ -1252,11 +1254,17 @@ xfs_link(
> >         if (error)
> >                 goto std_return;
> >  
> > +       if (xfs_has_parent(mp)) {
> > +               error = xfs_parent_init(mp, &parent);
> > +               if (error)
> > +                       goto std_return;
> > +       }
> > +
> >         resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
> 
> Forwarding on from the v2 series --
> 
> This patch ought to be modifying XFS_LINK_SPACE_RES so that each
> link()
> update reserves enough space to handle an expansion in the tdp
> directory
> (as it does now) *and* an expansion in the xattr structure of the sip
> child file.  This is how we avoid dipping into the free space reserve
> pool midway through a transaction, and avoid shutdowns when space is
> tight.
> 
> tr_res == space we reserve in the *log* to record updates.
> 
> XFS_LINK_SPACE_RES == block we reserve from the filesystem free space
> to
> handle expansions of metadata structures.
> 
> At this point in this version of the patchset, you've increased the
> log
> space reservations in anticipation of logging more information per
> transaction.  However, you've not increased the free space
> reservations
> to handle potential node splitting in the ondisk xattr btree.
> 
> (The rest of the patchset looks ok.)

Ok, looking at the later reviews, I'll add a similar helper function
here to use in place of XFS_LINK_SPACE_RES:

unsigned int
xfs_link_space_res(
         struct xfs_mount        *mp,
         unsigned int            namelen)
 {
         unsigned int            ret;
 
         ret = XFS_DIRENTER_SPACE_RES(mp, namelen);
         if (xfs_has_parent(mp))
                 ret += xfs_pptr_calc_space_res(mp, namelen);
 
         return ret;
 }

Thanks!
Allison
> 
> --D
> 
> >         error = xfs_trans_alloc_dir(tdp, &M_RES(mp)->tr_link, sip,
> > &resblks,
> >                         &tp, &nospace_error);
> >         if (error)
> > -               goto std_return;
> > +               goto drop_incompat;
> >  
> >         /*
> >          * If we are using project inheritance, we only allow hard
> > link
> > @@ -1289,14 +1297,27 @@ xfs_link(
> >         }
> >  
> >         error = xfs_dir_createname(tp, tdp, target_name, sip-
> > >i_ino,
> > -                                  resblks, NULL);
> > +                                  resblks, &diroffset);
> >         if (error)
> > -               goto error_return;
> > +               goto out_defer_cancel;
> >         xfs_trans_ichgtime(tp, tdp, XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> >         xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
> >  
> >         xfs_bumplink(tp, sip);
> >  
> > +       /*
> > +        * If we have parent pointers, we now need to add the
> > parent record to
> > +        * the attribute fork of the inode. If this is the initial
> > parent
> > +        * attribute, we need to create it correctly, otherwise we
> > can just add
> > +        * the parent to the inode.
> > +        */
> > +       if (parent) {
> > +               error = xfs_parent_defer_add(tp, parent, tdp,
> > target_name,
> > +                                            diroffset, sip);
> > +               if (error)
> > +                       goto out_defer_cancel;
> > +       }
> > +
> >         /*
> >          * If this is a synchronous mount, make sure that the
> >          * link transaction goes to disk before returning to
> > @@ -1310,11 +1331,16 @@ xfs_link(
> >         xfs_iunlock(sip, XFS_ILOCK_EXCL);
> >         return error;
> >  
> > - error_return:
> > +out_defer_cancel:
> > +       xfs_defer_cancel(tp);
> > +error_return:
> >         xfs_trans_cancel(tp);
> >         xfs_iunlock(tdp, XFS_ILOCK_EXCL);
> >         xfs_iunlock(sip, XFS_ILOCK_EXCL);
> > - std_return:
> > +drop_incompat:
> > +       if (parent)
> > +               xfs_parent_cancel(mp, parent);
> > +std_return:
> >         if (error == -ENOSPC && nospace_error)
> >                 error = nospace_error;
> >         return error;
> > -- 
> > 2.25.1
> >
Darrick J. Wong Sept. 26, 2022, 11:55 p.m. UTC | #3 
On Mon, Sep 26, 2022 at 09:49:10PM +0000, Allison Henderson wrote:
> On Fri, 2022-09-23 at 13:31 -0700, Darrick J. Wong wrote:
> > On Wed, Sep 21, 2022 at 10:44:47PM -0700,
> > allison.henderson@oracle.com wrote:
> > > From: Allison Henderson <allison.henderson@oracle.com>
> > > 
> > > This patch modifies xfs_link to add a parent pointer to the inode.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > ---
> > >  fs/xfs/xfs_inode.c | 44 +++++++++++++++++++++++++++++++++++-------
> > > --
> > >  1 file changed, 35 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 181d6417412e..af3f5edb7319 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -1228,14 +1228,16 @@ xfs_create_tmpfile(
> > >  
> > >  int
> > >  xfs_link(
> > > -       xfs_inode_t             *tdp,
> > > -       xfs_inode_t             *sip,
> > > +       struct xfs_inode        *tdp,
> > > +       struct xfs_inode        *sip,
> > >         struct xfs_name         *target_name)
> > >  {
> > > -       xfs_mount_t             *mp = tdp->i_mount;
> > > -       xfs_trans_t             *tp;
> > > +       struct xfs_mount        *mp = tdp->i_mount;
> > > +       struct xfs_trans        *tp;
> > >         int                     error, nospace_error = 0;
> > >         int                     resblks;
> > > +       xfs_dir2_dataptr_t      diroffset;
> > > +       struct xfs_parent_defer *parent = NULL;
> > >  
> > >         trace_xfs_link(tdp, target_name);
> > >  
> > > @@ -1252,11 +1254,17 @@ xfs_link(
> > >         if (error)
> > >                 goto std_return;
> > >  
> > > +       if (xfs_has_parent(mp)) {
> > > +               error = xfs_parent_init(mp, &parent);
> > > +               if (error)
> > > +                       goto std_return;
> > > +       }
> > > +
> > >         resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
> > 
> > Forwarding on from the v2 series --
> > 
> > This patch ought to be modifying XFS_LINK_SPACE_RES so that each
> > link()
> > update reserves enough space to handle an expansion in the tdp
> > directory
> > (as it does now) *and* an expansion in the xattr structure of the sip
> > child file.  This is how we avoid dipping into the free space reserve
> > pool midway through a transaction, and avoid shutdowns when space is
> > tight.
> > 
> > tr_res == space we reserve in the *log* to record updates.
> > 
> > XFS_LINK_SPACE_RES == block we reserve from the filesystem free space
> > to
> > handle expansions of metadata structures.
> > 
> > At this point in this version of the patchset, you've increased the
> > log
> > space reservations in anticipation of logging more information per
> > transaction.  However, you've not increased the free space
> > reservations
> > to handle potential node splitting in the ondisk xattr btree.
> > 
> > (The rest of the patchset looks ok.)
> 
> Ok, looking at the later reviews, I'll add a similar helper function
> here to use in place of XFS_LINK_SPACE_RES:
> 
> unsigned int
> xfs_link_space_res(
>          struct xfs_mount        *mp,
>          unsigned int            namelen)
>  {
>          unsigned int            ret;
>  
>          ret = XFS_DIRENTER_SPACE_RES(mp, namelen);
>          if (xfs_has_parent(mp))
>                  ret += xfs_pptr_calc_space_res(mp, namelen);
>  
>          return ret;
>  }

<nod>

--D

> Thanks!
> Allison
> > 
> > --D
> > 
> > >         error = xfs_trans_alloc_dir(tdp, &M_RES(mp)->tr_link, sip,
> > > &resblks,
> > >                         &tp, &nospace_error);
> > >         if (error)
> > > -               goto std_return;
> > > +               goto drop_incompat;
> > >  
> > >         /*
> > >          * If we are using project inheritance, we only allow hard
> > > link
> > > @@ -1289,14 +1297,27 @@ xfs_link(
> > >         }
> > >  
> > >         error = xfs_dir_createname(tp, tdp, target_name, sip-
> > > >i_ino,
> > > -                                  resblks, NULL);
> > > +                                  resblks, &diroffset);
> > >         if (error)
> > > -               goto error_return;
> > > +               goto out_defer_cancel;
> > >         xfs_trans_ichgtime(tp, tdp, XFS_ICHGTIME_MOD |
> > > XFS_ICHGTIME_CHG);
> > >         xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
> > >  
> > >         xfs_bumplink(tp, sip);
> > >  
> > > +       /*
> > > +        * If we have parent pointers, we now need to add the
> > > parent record to
> > > +        * the attribute fork of the inode. If this is the initial
> > > parent
> > > +        * attribute, we need to create it correctly, otherwise we
> > > can just add
> > > +        * the parent to the inode.
> > > +        */
> > > +       if (parent) {
> > > +               error = xfs_parent_defer_add(tp, parent, tdp,
> > > target_name,
> > > +                                            diroffset, sip);
> > > +               if (error)
> > > +                       goto out_defer_cancel;
> > > +       }
> > > +
> > >         /*
> > >          * If this is a synchronous mount, make sure that the
> > >          * link transaction goes to disk before returning to
> > > @@ -1310,11 +1331,16 @@ xfs_link(
> > >         xfs_iunlock(sip, XFS_ILOCK_EXCL);
> > >         return error;
> > >  
> > > - error_return:
> > > +out_defer_cancel:
> > > +       xfs_defer_cancel(tp);
> > > +error_return:
> > >         xfs_trans_cancel(tp);
> > >         xfs_iunlock(tdp, XFS_ILOCK_EXCL);
> > >         xfs_iunlock(sip, XFS_ILOCK_EXCL);
> > > - std_return:
> > > +drop_incompat:
> > > +       if (parent)
> > > +               xfs_parent_cancel(mp, parent);
> > > +std_return:
> > >         if (error == -ENOSPC && nospace_error)
> > >                 error = nospace_error;
> > >         return error;
> > > -- 
> > > 2.25.1
> > > 
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 181d6417412e..af3f5edb7319 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1228,14 +1228,16 @@  xfs_create_tmpfile(
 
 int
 xfs_link(
-	xfs_inode_t		*tdp,
-	xfs_inode_t		*sip,
+	struct xfs_inode	*tdp,
+	struct xfs_inode	*sip,
 	struct xfs_name		*target_name)
 {
-	xfs_mount_t		*mp = tdp->i_mount;
-	xfs_trans_t		*tp;
+	struct xfs_mount	*mp = tdp->i_mount;
+	struct xfs_trans	*tp;
 	int			error, nospace_error = 0;
 	int			resblks;
+	xfs_dir2_dataptr_t	diroffset;
+	struct xfs_parent_defer	*parent = NULL;
 
 	trace_xfs_link(tdp, target_name);
 
@@ -1252,11 +1254,17 @@  xfs_link(
 	if (error)
 		goto std_return;
 
+	if (xfs_has_parent(mp)) {
+		error = xfs_parent_init(mp, &parent);
+		if (error)
+			goto std_return;
+	}
+
 	resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
 	error = xfs_trans_alloc_dir(tdp, &M_RES(mp)->tr_link, sip, &resblks,
 			&tp, &nospace_error);
 	if (error)
-		goto std_return;
+		goto drop_incompat;
 
 	/*
 	 * If we are using project inheritance, we only allow hard link
@@ -1289,14 +1297,27 @@  xfs_link(
 	}
 
 	error = xfs_dir_createname(tp, tdp, target_name, sip->i_ino,
-				   resblks, NULL);
+				   resblks, &diroffset);
 	if (error)
-		goto error_return;
+		goto out_defer_cancel;
 	xfs_trans_ichgtime(tp, tdp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 	xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
 
 	xfs_bumplink(tp, sip);
 
+	/*
+	 * If we have parent pointers, we now need to add the parent record to
+	 * the attribute fork of the inode. If this is the initial parent
+	 * attribute, we need to create it correctly, otherwise we can just add
+	 * the parent to the inode.
+	 */
+	if (parent) {
+		error = xfs_parent_defer_add(tp, parent, tdp, target_name,
+					     diroffset, sip);
+		if (error)
+			goto out_defer_cancel;
+	}
+
 	/*
 	 * If this is a synchronous mount, make sure that the
 	 * link transaction goes to disk before returning to
@@ -1310,11 +1331,16 @@  xfs_link(
 	xfs_iunlock(sip, XFS_ILOCK_EXCL);
 	return error;
 
- error_return:
+out_defer_cancel:
+	xfs_defer_cancel(tp);
+error_return:
 	xfs_trans_cancel(tp);
 	xfs_iunlock(tdp, XFS_ILOCK_EXCL);
 	xfs_iunlock(sip, XFS_ILOCK_EXCL);
- std_return:
+drop_incompat:
+	if (parent)
+		xfs_parent_cancel(mp, parent);
+std_return:
 	if (error == -ENOSPC && nospace_error)
 		error = nospace_error;
 	return error;