[PATCH 0/3] xfs: rework xfs_inactive()

[PATCH 0/3] xfs: rework xfs_inactive()

[Brian Foster]

Hi all,

This set reworks the xfs_inactive() path with the intent to clean up the
transaction management overall. This is preparation work for the free
inode btree set and subsequent work in the area. The patches clean up
the remote symlink work, truncate work and ifree work respectively.

This passes through a quick xfstests run (with debug and lockdep)
without any major explosions. Thoughts appreciated.

Brian

Brian Foster (3):
  xfs: push down inactive transaction mgmt for remote symlinks
  xfs: push down inactive transaction mgmt for truncate
  xfs: push down inactive transaction mgmt for ifree

 fs/xfs/xfs_inode.c   | 212 ++++++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_symlink.c |  64 ++++++----------
 fs/xfs/xfs_symlink.h |   2 +-
 3 files changed, 147 insertions(+), 131 deletions(-)

-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Brian Foster]

Push down the transaction management for remote symlinks from
xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
cleaned up to avoid transaction management intended for the
calling context (i.e., trans duplication, reservation, item
attachment).

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_inode.c   | 15 ++++++------
 fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
 fs/xfs/xfs_symlink.h |  2 +-
 3 files changed, 31 insertions(+), 50 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e3d7538..30db70e 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1724,9 +1724,14 @@ xfs_inactive(
 	if (error)
 		return VN_INACTIVE_CACHE;
 
+	if (S_ISLNK(ip->i_d.di_mode)) {
+		error = xfs_inactive_symlink(ip);
+		if (error)
+			goto out;
+	}
+
 	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
-	resp = (truncate || S_ISLNK(ip->i_d.di_mode)) ?
-		&M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
+	resp = truncate ? &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
 
 	error = xfs_trans_reserve(tp, resp, 0, 0);
 	if (error) {
@@ -1738,11 +1743,7 @@ xfs_inactive(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	if (S_ISLNK(ip->i_d.di_mode)) {
-		error = xfs_inactive_symlink(ip, &tp);
-		if (error)
-			goto out_cancel;
-	} else if (truncate) {
+	if (truncate) {
 		ip->i_d.di_size = 0;
 		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index f622a97..f85f6f2 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -424,8 +424,7 @@ xfs_symlink(
  */
 STATIC int
 xfs_inactive_symlink_rmt(
-	xfs_inode_t	*ip,
-	xfs_trans_t	**tpp)
+	xfs_inode_t	*ip)
 {
 	xfs_buf_t	*bp;
 	int		committed;
@@ -437,11 +436,9 @@ xfs_inactive_symlink_rmt(
 	xfs_mount_t	*mp;
 	xfs_bmbt_irec_t	mval[XFS_SYMLINK_MAPS];
 	int		nmaps;
-	xfs_trans_t	*ntp;
 	int		size;
 	xfs_trans_t	*tp;
 
-	tp = *tpp;
 	mp = ip->i_mount;
 	ASSERT(ip->i_df.if_flags & XFS_IFEXTENTS);
 	/*
@@ -453,6 +450,14 @@ xfs_inactive_symlink_rmt(
 	 */
 	ASSERT(ip->i_d.di_nextents > 0 && ip->i_d.di_nextents <= 2);
 
+	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
+	if (error)
+		goto error0;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
 	/*
 	 * Lock the inode, fix the size, and join it to the transaction.
 	 * Hold it so in the normal path, we still have it locked for
@@ -471,7 +476,7 @@ xfs_inactive_symlink_rmt(
 	error = xfs_bmapi_read(ip, 0, xfs_symlink_blocks(mp, size),
 				mval, &nmaps, 0);
 	if (error)
-		goto error0;
+		goto error1;
 	/*
 	 * Invalidate the block(s). No validation is done.
 	 */
@@ -481,7 +486,7 @@ xfs_inactive_symlink_rmt(
 			XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
 		if (!bp) {
 			error = ENOMEM;
-			goto error1;
+			goto error2;
 		}
 		xfs_trans_binval(tp, bp);
 	}
@@ -490,13 +495,13 @@ xfs_inactive_symlink_rmt(
 	 */
 	if ((error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
 			&first_block, &free_list, &done)))
-		goto error1;
+		goto error2;
 	ASSERT(done);
 	/*
 	 * Commit the first transaction.  This logs the EFI and the inode.
 	 */
 	if ((error = xfs_bmap_finish(&tp, &free_list, &committed)))
-		goto error1;
+		goto error2;
 	/*
 	 * The transaction must have been committed, since there were
 	 * actually extents freed by xfs_bunmapi.  See xfs_bmap_finish.
@@ -508,29 +513,16 @@ xfs_inactive_symlink_rmt(
 	 * Mark it dirty so it will be logged and moved forward in the log as
 	 * part of every commit.
 	 */
-	xfs_trans_ijoin(tp, ip, 0);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 	/*
-	 * Get a new, empty transaction to return to our caller.
-	 */
-	ntp = xfs_trans_dup(tp);
-	/*
 	 * Commit the transaction containing extent freeing and EFDs.
-	 * If we get an error on the commit here or on the reserve below,
-	 * we need to unlock the inode since the new transaction doesn't
-	 * have the inode attached.
 	 */
-	error = xfs_trans_commit(tp, 0);
-	tp = ntp;
+	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
 	if (error) {
 		ASSERT(XFS_FORCED_SHUTDOWN(mp));
 		goto error0;
 	}
-	/*
-	 * transaction commit worked ok so we can drop the extra ticket
-	 * reference that we gained in xfs_trans_dup()
-	 */
-	xfs_log_ticket_put(tp->t_ticket);
 
 	/*
 	 * Remove the memory for extent descriptions (just bookkeeping).
@@ -538,23 +530,14 @@ xfs_inactive_symlink_rmt(
 	if (ip->i_df.if_bytes)
 		xfs_idata_realloc(ip, -ip->i_df.if_bytes, XFS_DATA_FORK);
 	ASSERT(ip->i_df.if_bytes == 0);
-	/*
-	 * Put an itruncate log reservation in the new transaction
-	 * for our caller.
-	 */
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
-	if (error) {
-		ASSERT(XFS_FORCED_SHUTDOWN(mp));
-		goto error0;
-	}
-
-	xfs_trans_ijoin(tp, ip, 0);
-	*tpp = tp;
 	return 0;
 
- error1:
+error2:
 	xfs_bmap_cancel(&free_list);
- error0:
+error1:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+error0:
+	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
 	return error;
 }
 
@@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
  */
 int
 xfs_inactive_symlink(
-	struct xfs_inode	*ip,
-	struct xfs_trans	**tp)
+	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	int			pathlen;
 
 	trace_xfs_inactive_symlink(ip);
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return XFS_ERROR(EIO);
 
@@ -599,5 +579,5 @@ xfs_inactive_symlink(
 	}
 
 	/* remove the remote symlink */
-	return xfs_inactive_symlink_rmt(ip, tp);
+	return xfs_inactive_symlink_rmt(ip);
 }
diff --git a/fs/xfs/xfs_symlink.h b/fs/xfs/xfs_symlink.h
index 99338ba..e75245d 100644
--- a/fs/xfs/xfs_symlink.h
+++ b/fs/xfs/xfs_symlink.h
@@ -22,6 +22,6 @@
 int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
 		const char *target_path, umode_t mode, struct xfs_inode **ipp);
 int xfs_readlink(struct xfs_inode *ip, char *link);
-int xfs_inactive_symlink(struct xfs_inode *ip, struct xfs_trans **tpp);
+int xfs_inactive_symlink(struct xfs_inode *ip);
 
 #endif /* __XFS_SYMLINK_H */
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Brian Foster]

On 09/18/2013 12:15 PM, Brian Foster wrote:
> Push down the transaction management for remote symlinks from
> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
> cleaned up to avoid transaction management intended for the
> calling context (i.e., trans duplication, reservation, item
> attachment).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c   | 15 ++++++------
>  fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
>  fs/xfs/xfs_symlink.h |  2 +-
>  3 files changed, 31 insertions(+), 50 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index f622a97..f85f6f2 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -424,8 +424,7 @@ xfs_symlink(
>   */
...
>  
> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>   */
>  int
>  xfs_inactive_symlink(
> -	struct xfs_inode	*ip,
> -	struct xfs_trans	**tp)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			pathlen;
>  
>  	trace_xfs_inactive_symlink(ip);
>  
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -

I just want to call out one thing here in case it isn't noticed on
review... the safety of this is something I was curious about.
Specifically, note that I've removed the inode locking from
xfs_inactive(), which previously covered xfs_inactive_symlink() (for
xfs_idata_realloc()), down into xfs_inactive_symlink_rmt().

My assumption was that this is currently ok since at this point we have
an inode with di_nlink == 0. If that's not accurate or not expected to
remain so after O_TMPFILE related work, I suppose I could pull the
locking back up into xfs_inactive_symlink().

Brian

>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);
>  
> @@ -599,5 +579,5 @@ xfs_inactive_symlink(
>  	}
>  
>  	/* remove the remote symlink */
> -	return xfs_inactive_symlink_rmt(ip, tp);
> +	return xfs_inactive_symlink_rmt(ip);
>  }
> diff --git a/fs/xfs/xfs_symlink.h b/fs/xfs/xfs_symlink.h
> index 99338ba..e75245d 100644
> --- a/fs/xfs/xfs_symlink.h
> +++ b/fs/xfs/xfs_symlink.h
> @@ -22,6 +22,6 @@
>  int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
>  		const char *target_path, umode_t mode, struct xfs_inode **ipp);
>  int xfs_readlink(struct xfs_inode *ip, char *link);
> -int xfs_inactive_symlink(struct xfs_inode *ip, struct xfs_trans **tpp);
> +int xfs_inactive_symlink(struct xfs_inode *ip);
>  
>  #endif /* __XFS_SYMLINK_H */
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Dave Chinner]

On Wed, Sep 18, 2013 at 02:06:26PM -0400, Brian Foster wrote:
> On 09/18/2013 12:15 PM, Brian Foster wrote:
> > Push down the transaction management for remote symlinks from
> > xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
> > cleaned up to avoid transaction management intended for the
> > calling context (i.e., trans duplication, reservation, item
> > attachment).
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c   | 15 ++++++------
> >  fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
> >  fs/xfs/xfs_symlink.h |  2 +-
> >  3 files changed, 31 insertions(+), 50 deletions(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> > index f622a97..f85f6f2 100644
> > --- a/fs/xfs/xfs_symlink.c
> > +++ b/fs/xfs/xfs_symlink.c
> > @@ -424,8 +424,7 @@ xfs_symlink(
> >   */
> ...
> >  
> > @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
> >   */
> >  int
> >  xfs_inactive_symlink(
> > -	struct xfs_inode	*ip,
> > -	struct xfs_trans	**tp)
> > +	struct xfs_inode	*ip)
> >  {
> >  	struct xfs_mount	*mp = ip->i_mount;
> >  	int			pathlen;
> >  
> >  	trace_xfs_inactive_symlink(ip);
> >  
> > -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> > -
> 
> I just want to call out one thing here in case it isn't noticed on
> review... the safety of this is something I was curious about.
> Specifically, note that I've removed the inode locking from
> xfs_inactive(), which previously covered xfs_inactive_symlink() (for
> xfs_idata_realloc()), down into xfs_inactive_symlink_rmt().

see my comments about idata_realloc() in the previous email. It
might be safe, but it's better not to leave a landmine if we add
some other caller to the function in the future.

> My assumption was that this is currently ok since at this point we have
> an inode with di_nlink == 0.

It's not an entirely correct assumption. The end result is likely
the same, but di_nlink has no influence here. i.e. the inode
lifecycle is rather complex and there is an interesting condition
that covers inodes going through xfs_inactive().

xfs_inactive() is called when the VFS reference count goes to zero
and the VFS inode is being reclaimed, but the XFS_IRECLAIMABLE flag
is not yet set on it. This doesn't happen until after xfs_inactive()
completes and the VFS calls ->destroy_inode. Hence the inode is in a
limbo state where calls to igrab() will fail but the inode can be
found in the inode radix trees without being marked as "under
reclaim conditions".

We handle this with xfs_iget_cache_hit() by the use of igrab(),
which will fail on such an inode, and we use the same logic in
xfs_inode_ag_walk_grab() to avoid this hole. That said,
xfs_reclaim_inode_grab() does no such thing - it only checks for
XFS_IRECLAIMABLE under an RCU traversal, and so may find inodes for
which that the radix tree reclaimable tag is stale. hence that
check is always done under a spinlock.

IOWs, the only thing that protects us from outside interference in
xfs_inactive() is the logic in the XFS inode cache lookups
specifically avoiding inodes in the transient state that
xfs_inactive() is called under. It doesn't matter what the contents
of the inode are - it's the safe transition from one lifecycle state
to the next that is important at this point.

So, like I said in the previous email, we have to be careful with
cache lookups to prevent races with the work xfs_inactive() is
doing, but that doesn't mean we shouldn't still lock the inodes
correctly when modifying them...

> If that's not accurate or not expected to
> remain so after O_TMPFILE related work, I suppose I could pull the
> locking back up into xfs_inactive_symlink().

O_TMPFILE itself won't change anything - they will look just like
any other unlinked inode going through xfs_inactive() on their way
to the XFS_IRECLAIMABLE state.

It's when we start separating the xfs_inactive() work into multiple
distinct stages to allow for optimisation of inode freeing that we
need to be careful as these introduce new states into the lifecycle
state machine. These will most likely involve new state flags and
radix tree tags and walks, but AFAICT, overall concept that
xfs_inactive/xfs_inactive_symlink is run from the same special
isolated "limbo" context should not change....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Brian Foster]

On 09/18/2013 06:51 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 02:06:26PM -0400, Brian Foster wrote:
>> On 09/18/2013 12:15 PM, Brian Foster wrote:
>>> Push down the transaction management for remote symlinks from
>>> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
>>> cleaned up to avoid transaction management intended for the
>>> calling context (i.e., trans duplication, reservation, item
>>> attachment).
>>>
>>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>>> ---
>>>  fs/xfs/xfs_inode.c   | 15 ++++++------
>>>  fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
>>>  fs/xfs/xfs_symlink.h |  2 +-
>>>  3 files changed, 31 insertions(+), 50 deletions(-)
>>>
>> ...
>>> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
>>> index f622a97..f85f6f2 100644
>>> --- a/fs/xfs/xfs_symlink.c
>>> +++ b/fs/xfs/xfs_symlink.c
>>> @@ -424,8 +424,7 @@ xfs_symlink(
>>>   */
>> ...
>>>  
>>> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>>>   */
>>>  int
>>>  xfs_inactive_symlink(
>>> -	struct xfs_inode	*ip,
>>> -	struct xfs_trans	**tp)
>>> +	struct xfs_inode	*ip)
>>>  {
>>>  	struct xfs_mount	*mp = ip->i_mount;
>>>  	int			pathlen;
>>>  
>>>  	trace_xfs_inactive_symlink(ip);
>>>  
>>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>> -
>>
>> I just want to call out one thing here in case it isn't noticed on
>> review... the safety of this is something I was curious about.
>> Specifically, note that I've removed the inode locking from
>> xfs_inactive(), which previously covered xfs_inactive_symlink() (for
>> xfs_idata_realloc()), down into xfs_inactive_symlink_rmt().
> 
> see my comments about idata_realloc() in the previous email. It
> might be safe, but it's better not to leave a landmine if we add
> some other caller to the function in the future.
> 
>> My assumption was that this is currently ok since at this point we have
>> an inode with di_nlink == 0.
> 
> It's not an entirely correct assumption. The end result is likely
> the same, but di_nlink has no influence here. i.e. the inode
> lifecycle is rather complex and there is an interesting condition
> that covers inodes going through xfs_inactive().
> 
> xfs_inactive() is called when the VFS reference count goes to zero
> and the VFS inode is being reclaimed, but the XFS_IRECLAIMABLE flag
> is not yet set on it. This doesn't happen until after xfs_inactive()
> completes and the VFS calls ->destroy_inode. Hence the inode is in a
> limbo state where calls to igrab() will fail but the inode can be
> found in the inode radix trees without being marked as "under
> reclaim conditions".
> 
> We handle this with xfs_iget_cache_hit() by the use of igrab(),
> which will fail on such an inode, and we use the same logic in
> xfs_inode_ag_walk_grab() to avoid this hole. That said,
> xfs_reclaim_inode_grab() does no such thing - it only checks for
> XFS_IRECLAIMABLE under an RCU traversal, and so may find inodes for
> which that the radix tree reclaimable tag is stale. hence that
> check is always done under a spinlock.
> 
> IOWs, the only thing that protects us from outside interference in
> xfs_inactive() is the logic in the XFS inode cache lookups
> specifically avoiding inodes in the transient state that
> xfs_inactive() is called under. It doesn't matter what the contents
> of the inode are - it's the safe transition from one lifecycle state
> to the next that is important at this point.
> 
> So, like I said in the previous email, we have to be careful with
> cache lookups to prevent races with the work xfs_inactive() is
> doing, but that doesn't mean we shouldn't still lock the inodes
> correctly when modifying them...
> 

So broadly speaking, the inode states are more granular than my di_nlink
based assumption. We have to account for access via internal caches,
even if the inode is in the process of being torn down in the vfs. I'll
have to wade through the caching code much more to understand the
intricacies. ;) Thanks for the breakdown.

With regard to the locking here, any preference as to whether
xfs_inactive_symlink() takes the lock and hands it to
xfs_inactive_symlink_rmt() or the former locks/unlocks and the latter
continues to work as implemented in this patch (save other comments to
be addressed)?

Actually now that I look at the code, xfs_inactive_symlink_rmt() does
the transaction allocation and reservation now, so for that reason I
think the lock/unlock pattern is required.

Brian

>> If that's not accurate or not expected to
>> remain so after O_TMPFILE related work, I suppose I could pull the
>> locking back up into xfs_inactive_symlink().
> 
> O_TMPFILE itself won't change anything - they will look just like
> any other unlinked inode going through xfs_inactive() on their way
> to the XFS_IRECLAIMABLE state.
> 
> It's when we start separating the xfs_inactive() work into multiple
> distinct stages to allow for optimisation of inode freeing that we
> need to be careful as these introduce new states into the lifecycle
> state machine. These will most likely involve new state flags and
> radix tree tags and walks, but AFAICT, overall concept that
> xfs_inactive/xfs_inactive_symlink is run from the same special
> isolated "limbo" context should not change....
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Dave Chinner]

On Thu, Sep 19, 2013 at 08:43:48AM -0400, Brian Foster wrote:
> On 09/18/2013 06:51 PM, Dave Chinner wrote:
> > On Wed, Sep 18, 2013 at 02:06:26PM -0400, Brian Foster wrote:
> >> I just want to call out one thing here in case it isn't noticed on
> >> review... the safety of this is something I was curious about.
> >> Specifically, note that I've removed the inode locking from
> >> xfs_inactive(), which previously covered xfs_inactive_symlink() (for
> >> xfs_idata_realloc()), down into xfs_inactive_symlink_rmt().
> > 
> > see my comments about idata_realloc() in the previous email. It
> > might be safe, but it's better not to leave a landmine if we add
> > some other caller to the function in the future.
> > 
> >> My assumption was that this is currently ok since at this point we have
> >> an inode with di_nlink == 0.
> > 
> > It's not an entirely correct assumption. The end result is likely
> > the same, but di_nlink has no influence here. i.e. the inode
> > lifecycle is rather complex and there is an interesting condition
> > that covers inodes going through xfs_inactive().
> > 
> > xfs_inactive() is called when the VFS reference count goes to zero
> > and the VFS inode is being reclaimed, but the XFS_IRECLAIMABLE flag
> > is not yet set on it. This doesn't happen until after xfs_inactive()
> > completes and the VFS calls ->destroy_inode. Hence the inode is in a
> > limbo state where calls to igrab() will fail but the inode can be
> > found in the inode radix trees without being marked as "under
> > reclaim conditions".
> > 
> > We handle this with xfs_iget_cache_hit() by the use of igrab(),
> > which will fail on such an inode, and we use the same logic in
> > xfs_inode_ag_walk_grab() to avoid this hole. That said,
> > xfs_reclaim_inode_grab() does no such thing - it only checks for
> > XFS_IRECLAIMABLE under an RCU traversal, and so may find inodes for
> > which that the radix tree reclaimable tag is stale. hence that
> > check is always done under a spinlock.
> > 
> > IOWs, the only thing that protects us from outside interference in
> > xfs_inactive() is the logic in the XFS inode cache lookups
> > specifically avoiding inodes in the transient state that
> > xfs_inactive() is called under. It doesn't matter what the contents
> > of the inode are - it's the safe transition from one lifecycle state
> > to the next that is important at this point.
> > 
> > So, like I said in the previous email, we have to be careful with
> > cache lookups to prevent races with the work xfs_inactive() is
> > doing, but that doesn't mean we shouldn't still lock the inodes
> > correctly when modifying them...
> > 
> 
> So broadly speaking, the inode states are more granular than my di_nlink
> based assumption.

More that the inode lifecycle states are not related to the value of
di_nlink at all ;)

> We have to account for access via internal caches,
> even if the inode is in the process of being torn down in the vfs. I'll
> have to wade through the caching code much more to understand the
> intricacies. ;) Thanks for the breakdown.

First you need to understand the VFS inode lifecycle, as the XFS
inode lifecycle mostly wraps around the outside of the VFS inode
lifecycle. ;)

> With regard to the locking here, any preference as to whether
> xfs_inactive_symlink() takes the lock and hands it to
> xfs_inactive_symlink_rmt() or the former locks/unlocks and the latter
> continues to work as implemented in this patch (save other comments to
> be addressed)?
> 
> Actually now that I look at the code, xfs_inactive_symlink_rmt() does
> the transaction allocation and reservation now, so for that reason I
> think the lock/unlock pattern is required.

Right, they are effectively two different cases with different
locking constraints.

Cheers,

Dave.

> 
> Brian
> 
> >> If that's not accurate or not expected to
> >> remain so after O_TMPFILE related work, I suppose I could pull the
> >> locking back up into xfs_inactive_symlink().
> > 
> > O_TMPFILE itself won't change anything - they will look just like
> > any other unlinked inode going through xfs_inactive() on their way
> > to the XFS_IRECLAIMABLE state.
> > 
> > It's when we start separating the xfs_inactive() work into multiple
> > distinct stages to allow for optimisation of inode freeing that we
> > need to be careful as these introduce new states into the lifecycle
> > state machine. These will most likely involve new state flags and
> > radix tree tags and walks, but AFAICT, overall concept that
> > xfs_inactive/xfs_inactive_symlink is run from the same special
> > isolated "limbo" context should not change....
> > 
> > Cheers,
> > 
> > Dave.
> > 
> 
> 

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Dave Chinner]

On Wed, Sep 18, 2013 at 12:15:58PM -0400, Brian Foster wrote:
> Push down the transaction management for remote symlinks from
> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
> cleaned up to avoid transaction management intended for the
> calling context (i.e., trans duplication, reservation, item
> attachment).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c   | 15 ++++++------
>  fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
>  fs/xfs/xfs_symlink.h |  2 +-
>  3 files changed, 31 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e3d7538..30db70e 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1724,9 +1724,14 @@ xfs_inactive(
>  	if (error)
>  		return VN_INACTIVE_CACHE;
>  
> +	if (S_ISLNK(ip->i_d.di_mode)) {
> +		error = xfs_inactive_symlink(ip);
> +		if (error)
> +			goto out;
> +	}
> +
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> -	resp = (truncate || S_ISLNK(ip->i_d.di_mode)) ?
> -		&M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
> +	resp = truncate ? &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
>  
>  	error = xfs_trans_reserve(tp, resp, 0, 0);
>  	if (error) {
> @@ -1738,11 +1743,7 @@ xfs_inactive(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> -	if (S_ISLNK(ip->i_d.di_mode)) {
> -		error = xfs_inactive_symlink(ip, &tp);
> -		if (error)
> -			goto out_cancel;
> -	} else if (truncate) {
> +	if (truncate) {
>  		ip->i_d.di_size = 0;
>  		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Just to maintain the same logic here, if it is a symlink shouldn't
we be ensuring that truncate is zero so we don't go down that path
(even if it's just an assert that you add)?

> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index f622a97..f85f6f2 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -424,8 +424,7 @@ xfs_symlink(
>   */
>  STATIC int
>  xfs_inactive_symlink_rmt(
> -	xfs_inode_t	*ip,
> -	xfs_trans_t	**tpp)
> +	xfs_inode_t	*ip)
>  {
>  	xfs_buf_t	*bp;
>  	int		committed;
> @@ -437,11 +436,9 @@ xfs_inactive_symlink_rmt(
>  	xfs_mount_t	*mp;
>  	xfs_bmbt_irec_t	mval[XFS_SYMLINK_MAPS];
>  	int		nmaps;
> -	xfs_trans_t	*ntp;
>  	int		size;
>  	xfs_trans_t	*tp;
>  
> -	tp = *tpp;
>  	mp = ip->i_mount;
>  	ASSERT(ip->i_df.if_flags & XFS_IFEXTENTS);
>  	/*
> @@ -453,6 +450,14 @@ xfs_inactive_symlink_rmt(
>  	 */
>  	ASSERT(ip->i_d.di_nextents > 0 && ip->i_d.di_nextents <= 2);
>  
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
> +	if (error)
> +		goto error0;

		goto error_trans_cancel;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
>  	/*
>  	 * Lock the inode, fix the size, and join it to the transaction.
>  	 * Hold it so in the normal path, we still have it locked for
> @@ -471,7 +476,7 @@ xfs_inactive_symlink_rmt(
>  	error = xfs_bmapi_read(ip, 0, xfs_symlink_blocks(mp, size),
>  				mval, &nmaps, 0);
>  	if (error)
> -		goto error0;
> +		goto error1;

		goto error_unlock;

>  	/*
>  	 * Invalidate the block(s). No validation is done.
>  	 */
> @@ -481,7 +486,7 @@ xfs_inactive_symlink_rmt(
>  			XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
>  		if (!bp) {
>  			error = ENOMEM;
> -			goto error1;
> +			goto error2;

			goto error_bmap_cancel;

>  		}
>  		xfs_trans_binval(tp, bp);
>  	}
> @@ -490,13 +495,13 @@ xfs_inactive_symlink_rmt(
>  	 */
>  	if ((error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
>  			&first_block, &free_list, &done)))
> -		goto error1;
> +		goto error2;

Can you convert this to:

	error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
			    &first_block, &free_list, &done);
	if (error)
		goto error_bmap_cancel;

>  	ASSERT(done);
>  	/*
>  	 * Commit the first transaction.  This logs the EFI and the inode.
>  	 */
>  	if ((error = xfs_bmap_finish(&tp, &free_list, &committed)))
> -		goto error1;
> +		goto error2;

Same here.

>  	/*
>  	 * The transaction must have been committed, since there were
>  	 * actually extents freed by xfs_bunmapi.  See xfs_bmap_finish.
> @@ -508,29 +513,16 @@ xfs_inactive_symlink_rmt(
>  	 * Mark it dirty so it will be logged and moved forward in the log as
>  	 * part of every commit.
>  	 */
> -	xfs_trans_ijoin(tp, ip, 0);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);

Oh, good, you caught the "need to unlock the inode at commit" case
:)

>  
> - error1:
> +error2:
>  	xfs_bmap_cancel(&free_list);
> - error0:
> +error1:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +error0:
> +	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
>  	return error;

And the error labels need reworking appropriately.

>  }
>  
> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>   */
>  int
>  xfs_inactive_symlink(
> -	struct xfs_inode	*ip,
> -	struct xfs_trans	**tp)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			pathlen;
>  
>  	trace_xfs_inactive_symlink(ip);
>  
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return XFS_ERROR(EIO);

The call to xfs_idata_realloc() needs to be done under the
XFS_ILOCK_EXCL here. We can race with other inode cache lookups
that do work, so we do need to ensure that we correctly lock
everything for modifications that are to be made to the inode state.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Brian Foster]

On 09/18/2013 06:17 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 12:15:58PM -0400, Brian Foster wrote:
>> Push down the transaction management for remote symlinks from
>> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
>> cleaned up to avoid transaction management intended for the
>> calling context (i.e., trans duplication, reservation, item
>> attachment).
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>>  fs/xfs/xfs_inode.c   | 15 ++++++------
>>  fs/xfs/xfs_symlink.c | 64 ++++++++++++++++++----------------------------------
>>  fs/xfs/xfs_symlink.h |  2 +-
>>  3 files changed, 31 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index e3d7538..30db70e 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1724,9 +1724,14 @@ xfs_inactive(
>>  	if (error)
>>  		return VN_INACTIVE_CACHE;
>>  
>> +	if (S_ISLNK(ip->i_d.di_mode)) {
>> +		error = xfs_inactive_symlink(ip);
>> +		if (error)
>> +			goto out;
>> +	}
>> +
>>  	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> -	resp = (truncate || S_ISLNK(ip->i_d.di_mode)) ?
>> -		&M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
>> +	resp = truncate ? &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
>>  
>>  	error = xfs_trans_reserve(tp, resp, 0, 0);
>>  	if (error) {
>> @@ -1738,11 +1743,7 @@ xfs_inactive(
>>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>>  	xfs_trans_ijoin(tp, ip, 0);
>>  
>> -	if (S_ISLNK(ip->i_d.di_mode)) {
>> -		error = xfs_inactive_symlink(ip, &tp);
>> -		if (error)
>> -			goto out_cancel;
>> -	} else if (truncate) {
>> +	if (truncate) {
>>  		ip->i_d.di_size = 0;
>>  		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> 
> Just to maintain the same logic here, if it is a symlink shouldn't
> we be ensuring that truncate is zero so we don't go down that path
> (even if it's just an assert that you add)?
> 

I missed this on my first pass of your review, but this actually turns
back into an if/else after patch 2. E.g., once we pull the truncate case
back before the transaction allocation still being used for the ifree
(until patch 3). So I think this is probably unnecessary.

Brian

>> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
>> index f622a97..f85f6f2 100644
>> --- a/fs/xfs/xfs_symlink.c
>> +++ b/fs/xfs/xfs_symlink.c
>> @@ -424,8 +424,7 @@ xfs_symlink(
>>   */
>>  STATIC int
>>  xfs_inactive_symlink_rmt(
>> -	xfs_inode_t	*ip,
>> -	xfs_trans_t	**tpp)
>> +	xfs_inode_t	*ip)
>>  {
>>  	xfs_buf_t	*bp;
>>  	int		committed;
>> @@ -437,11 +436,9 @@ xfs_inactive_symlink_rmt(
>>  	xfs_mount_t	*mp;
>>  	xfs_bmbt_irec_t	mval[XFS_SYMLINK_MAPS];
>>  	int		nmaps;
>> -	xfs_trans_t	*ntp;
>>  	int		size;
>>  	xfs_trans_t	*tp;
>>  
>> -	tp = *tpp;
>>  	mp = ip->i_mount;
>>  	ASSERT(ip->i_df.if_flags & XFS_IFEXTENTS);
>>  	/*
>> @@ -453,6 +450,14 @@ xfs_inactive_symlink_rmt(
>>  	 */
>>  	ASSERT(ip->i_d.di_nextents > 0 && ip->i_d.di_nextents <= 2);
>>  
>> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
>> +	if (error)
>> +		goto error0;
> 
> 		goto error_trans_cancel;
>> +
>> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +	xfs_trans_ijoin(tp, ip, 0);
>> +
>>  	/*
>>  	 * Lock the inode, fix the size, and join it to the transaction.
>>  	 * Hold it so in the normal path, we still have it locked for
>> @@ -471,7 +476,7 @@ xfs_inactive_symlink_rmt(
>>  	error = xfs_bmapi_read(ip, 0, xfs_symlink_blocks(mp, size),
>>  				mval, &nmaps, 0);
>>  	if (error)
>> -		goto error0;
>> +		goto error1;
> 
> 		goto error_unlock;
> 
>>  	/*
>>  	 * Invalidate the block(s). No validation is done.
>>  	 */
>> @@ -481,7 +486,7 @@ xfs_inactive_symlink_rmt(
>>  			XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
>>  		if (!bp) {
>>  			error = ENOMEM;
>> -			goto error1;
>> +			goto error2;
> 
> 			goto error_bmap_cancel;
> 
>>  		}
>>  		xfs_trans_binval(tp, bp);
>>  	}
>> @@ -490,13 +495,13 @@ xfs_inactive_symlink_rmt(
>>  	 */
>>  	if ((error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
>>  			&first_block, &free_list, &done)))
>> -		goto error1;
>> +		goto error2;
> 
> Can you convert this to:
> 
> 	error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
> 			    &first_block, &free_list, &done);
> 	if (error)
> 		goto error_bmap_cancel;
> 
>>  	ASSERT(done);
>>  	/*
>>  	 * Commit the first transaction.  This logs the EFI and the inode.
>>  	 */
>>  	if ((error = xfs_bmap_finish(&tp, &free_list, &committed)))
>> -		goto error1;
>> +		goto error2;
> 
> Same here.
> 
>>  	/*
>>  	 * The transaction must have been committed, since there were
>>  	 * actually extents freed by xfs_bunmapi.  See xfs_bmap_finish.
>> @@ -508,29 +513,16 @@ xfs_inactive_symlink_rmt(
>>  	 * Mark it dirty so it will be logged and moved forward in the log as
>>  	 * part of every commit.
>>  	 */
>> -	xfs_trans_ijoin(tp, ip, 0);
>> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> 
> Oh, good, you caught the "need to unlock the inode at commit" case
> :)
> 
>>  
>> - error1:
>> +error2:
>>  	xfs_bmap_cancel(&free_list);
>> - error0:
>> +error1:
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +error0:
>> +	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
>>  	return error;
> 
> And the error labels need reworking appropriately.
> 
>>  }
>>  
>> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>>   */
>>  int
>>  xfs_inactive_symlink(
>> -	struct xfs_inode	*ip,
>> -	struct xfs_trans	**tp)
>> +	struct xfs_inode	*ip)
>>  {
>>  	struct xfs_mount	*mp = ip->i_mount;
>>  	int			pathlen;
>>  
>>  	trace_xfs_inactive_symlink(ip);
>>  
>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> -
>>  	if (XFS_FORCED_SHUTDOWN(mp))
>>  		return XFS_ERROR(EIO);
> 
> The call to xfs_idata_realloc() needs to be done under the
> XFS_ILOCK_EXCL here. We can race with other inode cache lookups
> that do work, so we do need to ensure that we correctly lock
> everything for modifications that are to be made to the inode state.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: push down inactive transaction mgmt for remote symlinks

[Brian Foster]

On 09/18/2013 06:17 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 12:15:58PM -0400, Brian Foster wrote:
>> Push down the transaction management for remote symlinks from
>> xfs_inactive() down to xfs_inactive_symlink_rmt(). The latter is
>> cleaned up to avoid transaction management intended for the
>> calling context (i.e., trans duplication, reservation, item
>> attachment).
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
...
>> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
>> index f622a97..f85f6f2 100644
>> --- a/fs/xfs/xfs_symlink.c
>> +++ b/fs/xfs/xfs_symlink.c
>> @@ -424,8 +424,7 @@ xfs_symlink(
>>   */
>>  STATIC int
>>  xfs_inactive_symlink_rmt(
...
>>  	/*
>>  	 * The transaction must have been committed, since there were
>>  	 * actually extents freed by xfs_bunmapi.  See xfs_bmap_finish.
>> @@ -508,29 +513,16 @@ xfs_inactive_symlink_rmt(
>>  	 * Mark it dirty so it will be logged and moved forward in the log as
>>  	 * part of every commit.
>>  	 */
>> -	xfs_trans_ijoin(tp, ip, 0);
>> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> 
> Oh, good, you caught the "need to unlock the inode at commit" case
> :)
> 

Yeah, and while fixing the error handling order issues in v2 I just
noticed that this leaves the final xfs_idata_realloc() call in
xfs_inactive_symlink_rmt() unprotected wrt to the ilock. ;) I'll fix
that up to just do the (un)locking manually here as well for v3...

Brian

>>  
>> - error1:
>> +error2:
>>  	xfs_bmap_cancel(&free_list);
>> - error0:
>> +error1:
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +error0:
>> +	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
>>  	return error;
> 
> And the error labels need reworking appropriately.
> 
>>  }
>>  
>> @@ -563,16 +546,13 @@ xfs_inactive_symlink_rmt(
>>   */
>>  int
>>  xfs_inactive_symlink(
>> -	struct xfs_inode	*ip,
>> -	struct xfs_trans	**tp)
>> +	struct xfs_inode	*ip)
>>  {
>>  	struct xfs_mount	*mp = ip->i_mount;
>>  	int			pathlen;
>>  
>>  	trace_xfs_inactive_symlink(ip);
>>  
>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> -
>>  	if (XFS_FORCED_SHUTDOWN(mp))
>>  		return XFS_ERROR(EIO);
> 
> The call to xfs_idata_realloc() needs to be done under the
> XFS_ILOCK_EXCL here. We can race with other inode cache lookups
> that do work, so we do need to ensure that we correctly lock
> everything for modifications that are to be made to the inode state.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/3] xfs: push down inactive transaction mgmt for truncate

[Brian Foster]

Create the new xfs_inactive_truncate() function to handle the
truncate portion of xfs_inactive(). Push the locking and
transaction management into the new function.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_inode.c | 112 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 63 insertions(+), 49 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 30db70e..9416462 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1663,6 +1663,53 @@ xfs_release(
 }
 
 /*
+ * xfs_inactive_truncate
+ *
+ * Called to perform a truncate when an inode becomes unlinked.
+ */
+STATIC int
+xfs_inactive_truncate(
+	struct xfs_inode *ip)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
+
+	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
+
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
+	if (error) {
+		ASSERT(XFS_FORCED_SHUTDOWN(mp));
+		goto error0;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	ip->i_d.di_size = 0;
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
+	error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
+	if (error)
+		goto error1;
+
+	ASSERT(ip->i_d.di_nextents == 0);
+
+	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+	if (error)
+		goto error1;
+
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return 0;
+
+error1:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+error0:
+	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
+	return error;
+}
+
+/*
  * xfs_inactive
  *
  * This is called when the vnode reference count for the vnode
@@ -1679,7 +1726,6 @@ xfs_inactive(
 	int			committed;
 	struct xfs_trans	*tp;
 	struct xfs_mount	*mp;
-	struct xfs_trans_res	*resp;
 	int			error;
 	int			truncate = 0;
 
@@ -1724,35 +1770,12 @@ xfs_inactive(
 	if (error)
 		return VN_INACTIVE_CACHE;
 
-	if (S_ISLNK(ip->i_d.di_mode)) {
+	if (S_ISLNK(ip->i_d.di_mode))
 		error = xfs_inactive_symlink(ip);
-		if (error)
-			goto out;
-	}
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
-	resp = truncate ? &M_RES(mp)->tr_itruncate : &M_RES(mp)->tr_ifree;
-
-	error = xfs_trans_reserve(tp, resp, 0, 0);
-	if (error) {
-		ASSERT(XFS_FORCED_SHUTDOWN(mp));
-		xfs_trans_cancel(tp, 0);
-		return VN_INACTIVE_CACHE;
-	}
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-
-	if (truncate) {
-		ip->i_d.di_size = 0;
-		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
-
-		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
-		if (error)
-			goto out_cancel;
-
-		ASSERT(ip->i_d.di_nextents == 0);
-	}
+	else if (truncate)
+		error = xfs_inactive_truncate(ip);
+	if (error)
+		goto out;
 
 	/*
 	 * If there are attributes associated with the file then blow them away
@@ -1763,25 +1786,9 @@ xfs_inactive(
 	if (ip->i_d.di_anextents > 0) {
 		ASSERT(ip->i_d.di_forkoff != 0);
 
-		error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-		if (error)
-			goto out_unlock;
-
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-
 		error = xfs_attr_inactive(ip);
 		if (error)
 			goto out;
-
-		tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
-		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
-		if (error) {
-			xfs_trans_cancel(tp, 0);
-			goto out;
-		}
-
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-		xfs_trans_ijoin(tp, ip, 0);
 	}
 
 	if (ip->i_afp)
@@ -1789,6 +1796,17 @@ xfs_inactive(
 
 	ASSERT(ip->i_d.di_anextents == 0);
 
+	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
+	if (error) {
+		ASSERT(XFS_FORCED_SHUTDOWN(mp));
+		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
+		goto out;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
 	/*
 	 * Free the inode.
 	 */
@@ -1831,13 +1849,9 @@ xfs_inactive(
 	 * Release the dquots held by inode, if any.
 	 */
 	xfs_qm_dqdetach(ip);
-out_unlock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out:
 	return VN_INACTIVE_CACHE;
-out_cancel:
-	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
-	goto out_unlock;
 }
 
 /*
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/3] xfs: push down inactive transaction mgmt for truncate

[Dave Chinner]

On Wed, Sep 18, 2013 at 12:15:59PM -0400, Brian Foster wrote:
> Create the new xfs_inactive_truncate() function to handle the
> truncate portion of xfs_inactive(). Push the locking and
> transaction management into the new function.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 112 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 63 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 30db70e..9416462 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1663,6 +1663,53 @@ xfs_release(
>  }
>  
>  /*
> + * xfs_inactive_truncate
> + *
> + * Called to perform a truncate when an inode becomes unlinked.
> + */
> +STATIC int
> +xfs_inactive_truncate(
> +	struct xfs_inode *ip)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> +
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
> +	if (error) {
> +		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> +		goto error0;

error_trans_cancel...

> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	ip->i_d.di_size = 0;
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

Can you add a comment here that we are logging the inode size so
that if the system crashes part way through the truncation we don't
need to worry about stale data exposure?  There's a similar, more
expansive comment in xfs_setattr_size() - maybe a quick one-line
explaination and a "see setattr_size for more info" woul dbe
sufficient.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

[Brian Foster]

Push the inode free work performed during xfs_inactive() down into
a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
all inode locking and transaction management more directly
associated with freeing the inode xattrs, extents and the inode
itself.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_inode.c | 121 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 71 insertions(+), 50 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 9416462..a6ed69d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1710,6 +1710,74 @@ error0:
 }
 
 /*
+ * xfs_inactive_ifree()
+ *
+ * Perform the inode free when an inode is unlinked.
+ */
+STATIC int
+xfs_inactive_ifree(
+	struct xfs_inode *ip)
+{
+	xfs_bmap_free_t		free_list;
+	xfs_fsblock_t		first_block;
+	int			committed;
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
+
+	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
+	if (error) {
+		ASSERT(XFS_FORCED_SHUTDOWN(mp));
+		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
+		return error;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	xfs_bmap_init(&free_list, &first_block);
+	error = xfs_ifree(tp, ip, &free_list);
+	if (error) {
+		/*
+		 * If we fail to free the inode, shut down.  The cancel
+		 * might do that, we need to make sure.  Otherwise the
+		 * inode might be lost for a long time or forever.
+		 */
+		if (!XFS_FORCED_SHUTDOWN(mp)) {
+			xfs_notice(mp, "%s: xfs_ifree returned error %d",
+				__func__, error);
+			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
+		}
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
+		return error;
+	}
+
+	/*
+	 * Credit the quota account(s). The inode is gone.
+	 */
+	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
+
+	/*
+	 * Just ignore errors at this point.  There is nothing we can
+	 * do except to try to keep going. Make sure it's not a silent
+	 * error.
+	 */
+	error = xfs_bmap_finish(&tp,  &free_list, &committed);
+	if (error)
+		xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
+			__func__, error);
+	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+	if (error)
+		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
+			__func__, error);
+
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return 0;
+}
+
+/*
  * xfs_inactive
  *
  * This is called when the vnode reference count for the vnode
@@ -1721,10 +1789,6 @@ int
 xfs_inactive(
 	xfs_inode_t	*ip)
 {
-	xfs_bmap_free_t		free_list;
-	xfs_fsblock_t		first_block;
-	int			committed;
-	struct xfs_trans	*tp;
 	struct xfs_mount	*mp;
 	int			error;
 	int			truncate = 0;
@@ -1796,60 +1860,17 @@ xfs_inactive(
 
 	ASSERT(ip->i_d.di_anextents == 0);
 
-	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
-	if (error) {
-		ASSERT(XFS_FORCED_SHUTDOWN(mp));
-		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
-		goto out;
-	}
-
-	xfs_ilock(ip, XFS_ILOCK_EXCL);
-	xfs_trans_ijoin(tp, ip, 0);
-
 	/*
 	 * Free the inode.
 	 */
-	xfs_bmap_init(&free_list, &first_block);
-	error = xfs_ifree(tp, ip, &free_list);
-	if (error) {
-		/*
-		 * If we fail to free the inode, shut down.  The cancel
-		 * might do that, we need to make sure.  Otherwise the
-		 * inode might be lost for a long time or forever.
-		 */
-		if (!XFS_FORCED_SHUTDOWN(mp)) {
-			xfs_notice(mp, "%s: xfs_ifree returned error %d",
-				__func__, error);
-			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
-		}
-		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
-	} else {
-		/*
-		 * Credit the quota account(s). The inode is gone.
-		 */
-		xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
-
-		/*
-		 * Just ignore errors at this point.  There is nothing we can
-		 * do except to try to keep going. Make sure it's not a silent
-		 * error.
-		 */
-		error = xfs_bmap_finish(&tp,  &free_list, &committed);
-		if (error)
-			xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
-				__func__, error);
-		error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
-		if (error)
-			xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
-				__func__, error);
-	}
+	error = xfs_inactive_ifree(ip);
+	if (error)
+		goto out;
 
 	/*
 	 * Release the dquots held by inode, if any.
 	 */
 	xfs_qm_dqdetach(ip);
-	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 out:
 	return VN_INACTIVE_CACHE;
 }
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

[Dave Chinner]

On Wed, Sep 18, 2013 at 12:16:00PM -0400, Brian Foster wrote:
> Push the inode free work performed during xfs_inactive() down into
> a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
> all inode locking and transaction management more directly
> associated with freeing the inode xattrs, extents and the inode
> itself.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 121 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 71 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 9416462..a6ed69d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1710,6 +1710,74 @@ error0:
>  }
>  
>  /*
> + * xfs_inactive_ifree()
> + *
> + * Perform the inode free when an inode is unlinked.
> + */
> +STATIC int
> +xfs_inactive_ifree(
> +	struct xfs_inode *ip)
> +{
> +	xfs_bmap_free_t		free_list;
> +	xfs_fsblock_t		first_block;
> +	int			committed;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
> +	if (error) {
> +		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	xfs_bmap_init(&free_list, &first_block);
> +	error = xfs_ifree(tp, ip, &free_list);
> +	if (error) {
> +		/*
> +		 * If we fail to free the inode, shut down.  The cancel
> +		 * might do that, we need to make sure.  Otherwise the
> +		 * inode might be lost for a long time or forever.
> +		 */
> +		if (!XFS_FORCED_SHUTDOWN(mp)) {
> +			xfs_notice(mp, "%s: xfs_ifree returned error %d",
> +				__func__, error);
> +			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> +		}
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
> +		return error;
> +	}
> +
> +	/*
> +	 * Credit the quota account(s). The inode is gone.
> +	 */
> +	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
> +
> +	/*
> +	 * Just ignore errors at this point.  There is nothing we can
> +	 * do except to try to keep going. Make sure it's not a silent
> +	 * error.
> +	 */
> +	error = xfs_bmap_finish(&tp,  &free_list, &committed);
> +	if (error)
> +		xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
> +			__func__, error);
> +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> +	if (error)
> +		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
> +			__func__, error);
> +
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return 0;
> +}

I suspect we can clean up the error handling here now, and make it
look like the symlink remove inactive handle where we cancel bmaps
and abort transactions and trigger shutdowns appropriately. I'd
leave that to a separate patchset, though ;)

> -				__func__, error);
> -	}
> +	error = xfs_inactive_ifree(ip);
> +	if (error)
> +		goto out;
>  
>  	/*
>  	 * Release the dquots held by inode, if any.
>  	 */
>  	xfs_qm_dqdetach(ip);
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out:
>  	return VN_INACTIVE_CACHE;
>  }

I think it's time to kill VN_INACTIVE_CACHE, as well. It's a
hold-over from Irix, and it serves no purpose what-so-ever on
Linux...

Otherwise it looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

[Brian Foster]

On 09/18/2013 07:06 PM, Dave Chinner wrote:
> On Wed, Sep 18, 2013 at 12:16:00PM -0400, Brian Foster wrote:
>> Push the inode free work performed during xfs_inactive() down into
>> a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
>> all inode locking and transaction management more directly
>> associated with freeing the inode xattrs, extents and the inode
>> itself.
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>>  fs/xfs/xfs_inode.c | 121 +++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 71 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 9416462..a6ed69d 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1710,6 +1710,74 @@ error0:
>>  }
>>  
>>  /*
>> + * xfs_inactive_ifree()
>> + *
>> + * Perform the inode free when an inode is unlinked.
>> + */
>> +STATIC int
>> +xfs_inactive_ifree(
>> +	struct xfs_inode *ip)
>> +{
>> +	xfs_bmap_free_t		free_list;
>> +	xfs_fsblock_t		first_block;
>> +	int			committed;
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	struct xfs_trans	*tp;
>> +	int			error;
>> +
>> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
>> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
>> +	if (error) {
>> +		ASSERT(XFS_FORCED_SHUTDOWN(mp));
>> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
>> +		return error;
>> +	}
>> +
>> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +	xfs_trans_ijoin(tp, ip, 0);
>> +
>> +	xfs_bmap_init(&free_list, &first_block);
>> +	error = xfs_ifree(tp, ip, &free_list);
>> +	if (error) {
>> +		/*
>> +		 * If we fail to free the inode, shut down.  The cancel
>> +		 * might do that, we need to make sure.  Otherwise the
>> +		 * inode might be lost for a long time or forever.
>> +		 */
>> +		if (!XFS_FORCED_SHUTDOWN(mp)) {
>> +			xfs_notice(mp, "%s: xfs_ifree returned error %d",
>> +				__func__, error);
>> +			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
>> +		}
>> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>> +		return error;
>> +	}
>> +
>> +	/*
>> +	 * Credit the quota account(s). The inode is gone.
>> +	 */
>> +	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
>> +
>> +	/*
>> +	 * Just ignore errors at this point.  There is nothing we can
>> +	 * do except to try to keep going. Make sure it's not a silent
>> +	 * error.
>> +	 */
>> +	error = xfs_bmap_finish(&tp,  &free_list, &committed);
>> +	if (error)
>> +		xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
>> +			__func__, error);
>> +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
>> +	if (error)
>> +		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
>> +			__func__, error);
>> +
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +	return 0;
>> +}
> 
> I suspect we can clean up the error handling here now, and make it
> look like the symlink remove inactive handle where we cancel bmaps
> and abort transactions and trigger shutdowns appropriately. I'd
> leave that to a separate patchset, though ;)
> 

Hmm, well I follow what you mean as far as changing the code I think.
But what changed that makes this safe? Or are you suggesting to shutdown
on a bmap_finish/trans_commit failure instead of just "being noisy?"

(Regardless, a separate patchset sounds good..)

>> -				__func__, error);
>> -	}
>> +	error = xfs_inactive_ifree(ip);
>> +	if (error)
>> +		goto out;
>>  
>>  	/*
>>  	 * Release the dquots held by inode, if any.
>>  	 */
>>  	xfs_qm_dqdetach(ip);
>> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>>  out:
>>  	return VN_INACTIVE_CACHE;
>>  }
> 
> I think it's time to kill VN_INACTIVE_CACHE, as well. It's a
> hold-over from Irix, and it serves no purpose what-so-ever on
> Linux...
> 

Ok, looks like a trivial patch to append to the series. In fact, there
appears to be no reason to return a value from xfs_inactive() at all
(along with the out label being unnecessary)... Thanks.

Brian

> Otherwise it looks good.
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

[Dave Chinner]

On Thu, Sep 19, 2013 at 08:44:06AM -0400, Brian Foster wrote:
> On 09/18/2013 07:06 PM, Dave Chinner wrote:
> > On Wed, Sep 18, 2013 at 12:16:00PM -0400, Brian Foster wrote:
> >> Push the inode free work performed during xfs_inactive() down into
> >> a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
> >> all inode locking and transaction management more directly
> >> associated with freeing the inode xattrs, extents and the inode
> >> itself.
.....
> >> +	/*
> >> +	 * Credit the quota account(s). The inode is gone.
> >> +	 */
> >> +	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
> >> +
> >> +	/*
> >> +	 * Just ignore errors at this point.  There is nothing we can
> >> +	 * do except to try to keep going. Make sure it's not a silent
> >> +	 * error.
> >> +	 */
> >> +	error = xfs_bmap_finish(&tp,  &free_list, &committed);
> >> +	if (error)
> >> +		xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
> >> +			__func__, error);
> >> +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> >> +	if (error)
> >> +		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
> >> +			__func__, error);
> >> +
> >> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >> +	return 0;
> >> +}
> > 
> > I suspect we can clean up the error handling here now, and make it
> > look like the symlink remove inactive handle where we cancel bmaps
> > and abort transactions and trigger shutdowns appropriately. I'd
> > leave that to a separate patchset, though ;)
> > 
> 
> Hmm, well I follow what you mean as far as changing the code I think.
> But what changed that makes this safe? Or are you suggesting to shutdown
> on a bmap_finish/trans_commit failure instead of just "being noisy?"

I was thinking aloud. The error handling there comes from a static
checker that pointed out that we weren't handling the errors at all
in this code (along with another 50 or so other error handling
problems). So rather than change the code logic at the time (because
I didn't understand why the code ignored the errors) I simply made
failures noisy so we'd find out if there were real errors being
ignored there.

Now we know that errors in this code path are extremely rare
(nobody has ever reported a failure with these errors in them) so
it's probably time to convert them to do the correct thing when
xfs_bmap_finish() operations fail: abort transactions and
potentially shut the filesystem down....

> (Regardless, a separate patchset sounds good..)

*nod*

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs