* [PATCH v2 1/4] xfs: push down inactive transaction mgmt for remote symlinks
2013-09-19 19:15 [PATCH v2 0/4] xfs: rework xfs_inactive() Brian Foster
@ 2013-09-19 19:15 ` Brian Foster
2013-09-19 23:42 ` Dave Chinner
2013-09-19 19:15 ` [PATCH v2 2/4] xfs: push down inactive transaction mgmt for truncate Brian Foster
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Brian Foster @ 2013-09-19 19:15 UTC (permalink / raw)
To: xfs
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 | 81 ++++++++++++++++++++++------------------------------
fs/xfs/xfs_symlink.h | 2 +-
3 files changed, 43 insertions(+), 55 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..2ce31a5 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 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 error_unlock;
/*
* Invalidate the block(s). No validation is done.
*/
@@ -481,22 +486,24 @@ xfs_inactive_symlink_rmt(
XFS_FSB_TO_BB(mp, mval[i].br_blockcount), 0);
if (!bp) {
error = ENOMEM;
- goto error1;
+ goto error_bmap_cancel;
}
xfs_trans_binval(tp, bp);
}
/*
* Unmap the dead block(s) to the free_list.
*/
- if ((error = xfs_bunmapi(tp, ip, 0, size, XFS_BMAPI_METADATA, nmaps,
- &first_block, &free_list, &done)))
- goto error1;
+ 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;
+ error = xfs_bmap_finish(&tp, &free_list, &committed);
+ if (error)
+ goto error_bmap_cancel;
/*
* The transaction must have been committed, since there were
* actually extents freed by xfs_bunmapi. See xfs_bmap_finish.
@@ -508,29 +515,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;
+ goto error_trans_cancel;
}
- /*
- * 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 +532,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:
+error_bmap_cancel:
xfs_bmap_cancel(&free_list);
- error0:
+error_unlock:
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+error_trans_cancel:
+ xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
return error;
}
@@ -563,16 +548,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);
@@ -590,14 +572,19 @@ xfs_inactive_symlink(
return XFS_ERROR(EFSCORRUPTED);
}
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+
if (ip->i_df.if_flags & XFS_IFINLINE) {
- if (ip->i_df.if_bytes > 0)
+ if (ip->i_df.if_bytes > 0)
xfs_idata_realloc(ip, -(ip->i_df.if_bytes),
XFS_DATA_FORK);
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
ASSERT(ip->i_df.if_bytes == 0);
return 0;
}
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
/* 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] xfs: push down inactive transaction mgmt for remote symlinks
2013-09-19 19:15 ` [PATCH v2 1/4] xfs: push down inactive transaction mgmt for remote symlinks Brian Foster
@ 2013-09-19 23:42 ` Dave Chinner
0 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2013-09-19 23:42 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
On Thu, Sep 19, 2013 at 03:15:18PM -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>
Couple of things....
> index f622a97..2ce31a5 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)
struct xfs_inode
>
> + tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> + error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
> + if (error)
> + goto error_trans_cancel;
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, 0);
Ok, here's where naming the jump labels sanely points out problems.
We've locked and joined the inode to the transaction. That means we
can't unlock the inode until *after* we've committed or aborted the
transaction. Unlocking it before the abort mens someone else can
lock it into a transaction and modify it before the abort is
processed....
That means the error handling stack is doing things the wrong way
around.
To fix it, I'd get rid of the error_trans_cancel label and
just cancel the transaction directly if xfs_trans_reserve() fails.
That doesn't need the abort flag, as nothing has been added to the
transaction at that point.
Then the "error_unlock" case can be converted to cancel the
transaction and then unlock the inode (maybe rename that case to
"error_trans_cancel").
> @@ -508,29 +515,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;
> + goto error_trans_cancel;
There's not need to cancel a transaction on a commit error. An
error in the commit will run an abort/cancel before it returns, and
as such the transaction handle is always unusable on return from
xfs_trans_commit()...
> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -
> if (XFS_FORCED_SHUTDOWN(mp))
> return XFS_ERROR(EIO);
>
> @@ -590,14 +572,19 @@ xfs_inactive_symlink(
> return XFS_ERROR(EFSCORRUPTED);
> }
>
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
You should lock before checking the size of the inode.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] xfs: push down inactive transaction mgmt for truncate
2013-09-19 19:15 [PATCH v2 0/4] xfs: rework xfs_inactive() Brian Foster
2013-09-19 19:15 ` [PATCH v2 1/4] xfs: push down inactive transaction mgmt for remote symlinks Brian Foster
@ 2013-09-19 19:15 ` Brian Foster
2013-09-19 23:47 ` Dave Chinner
2013-09-19 19:15 ` [PATCH v2 3/4] xfs: push down inactive transaction mgmt for ifree Brian Foster
2013-09-19 19:15 ` [PATCH v2 4/4] xfs: clean up xfs_inactive() error handling, kill VN_INACTIVE_[NO]CACHE Brian Foster
3 siblings, 1 reply; 9+ messages in thread
From: Brian Foster @ 2013-09-19 19:15 UTC (permalink / raw)
To: xfs
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 | 117 +++++++++++++++++++++++++++++++----------------------
1 file changed, 68 insertions(+), 49 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 30db70e..33bb9be 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1663,6 +1663,58 @@ 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 error_trans_cancel;
+ }
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, 0);
+
+ /*
+ * Log the inode size first to prevent stale data exposure in the event
+ * of a system crash before the truncate completes. See the related
+ * comment in xfs_setattr_size() for details.
+ */
+ 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 error_unlock;
+
+ ASSERT(ip->i_d.di_nextents == 0);
+
+ error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+ if (error)
+ goto error_unlock;
+
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ return 0;
+
+error_unlock:
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+error_trans_cancel:
+ 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 +1731,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 +1775,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 +1791,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 +1801,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 +1854,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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/4] xfs: push down inactive transaction mgmt for truncate
2013-09-19 19:15 ` [PATCH v2 2/4] xfs: push down inactive transaction mgmt for truncate Brian Foster
@ 2013-09-19 23:47 ` Dave Chinner
0 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2013-09-19 23:47 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
On Thu, Sep 19, 2013 at 03:15:19PM -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 | 117 +++++++++++++++++++++++++++++++----------------------
> 1 file changed, 68 insertions(+), 49 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 30db70e..33bb9be 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1663,6 +1663,58 @@ 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 error_trans_cancel;
> + }
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, 0);
> +
> + /*
> + * Log the inode size first to prevent stale data exposure in the event
> + * of a system crash before the truncate completes. See the related
> + * comment in xfs_setattr_size() for details.
> + */
> + 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 error_unlock;
> +
> + ASSERT(ip->i_d.di_nextents == 0);
> +
> + error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> + if (error)
> + goto error_unlock;
Same again - no cancel on commit error, just fall through and return
the error....
> +
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> + return 0;
> +
> +error_unlock:
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +error_trans_cancel:
> + xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
Same again w.r.t. unlock/cancel. FWIW, see xfs_free_file_space() for
an example of how the ordering is supposed to work.
And, note:
> - 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);
Different flags :)
> @@ -1831,13 +1854,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;
And the ordering of the previous code... :)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] xfs: push down inactive transaction mgmt for ifree
2013-09-19 19:15 [PATCH v2 0/4] xfs: rework xfs_inactive() Brian Foster
2013-09-19 19:15 ` [PATCH v2 1/4] xfs: push down inactive transaction mgmt for remote symlinks Brian Foster
2013-09-19 19:15 ` [PATCH v2 2/4] xfs: push down inactive transaction mgmt for truncate Brian Foster
@ 2013-09-19 19:15 ` Brian Foster
2013-09-19 23:49 ` Dave Chinner
2013-09-19 19:15 ` [PATCH v2 4/4] xfs: clean up xfs_inactive() error handling, kill VN_INACTIVE_[NO]CACHE Brian Foster
3 siblings, 1 reply; 9+ messages in thread
From: Brian Foster @ 2013-09-19 19:15 UTC (permalink / raw)
To: xfs
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 33bb9be..dde182e 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1715,6 +1715,74 @@ error_trans_cancel:
}
/*
+ * 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
@@ -1726,10 +1794,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;
@@ -1801,60 +1865,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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 3/4] xfs: push down inactive transaction mgmt for ifree
2013-09-19 19:15 ` [PATCH v2 3/4] xfs: push down inactive transaction mgmt for ifree Brian Foster
@ 2013-09-19 23:49 ` Dave Chinner
0 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2013-09-19 23:49 UTC (permalink / raw)
To: Brian Foster; +Cc: xfs
On Thu, Sep 19, 2013 at 03:15:20PM -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>
Looks good. I'll wait for the fixed versions of the other patches
so i can test it before signing off on it. ;)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] xfs: clean up xfs_inactive() error handling, kill VN_INACTIVE_[NO]CACHE
2013-09-19 19:15 [PATCH v2 0/4] xfs: rework xfs_inactive() Brian Foster
` (2 preceding siblings ...)
2013-09-19 19:15 ` [PATCH v2 3/4] xfs: push down inactive transaction mgmt for ifree Brian Foster
@ 2013-09-19 19:15 ` Brian Foster
2013-09-19 23:50 ` Dave Chinner
3 siblings, 1 reply; 9+ messages in thread
From: Brian Foster @ 2013-09-19 19:15 UTC (permalink / raw)
To: xfs
The xfs_inactive() return value is meaningless. Turn xfs_inactive()
into a void function and clean up the error handling appropriately.
Kill the VN_INACTIVE_[NO]CACHE directives as they are not relevant
to Linux.
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_inode.c | 28 +++++++++++-----------------
fs/xfs/xfs_inode.h | 2 +-
fs/xfs/xfs_vnode.h | 8 --------
3 files changed, 12 insertions(+), 26 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index dde182e..6b08fd2 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1790,7 +1790,7 @@ xfs_inactive_ifree(
* now be truncated. Also, we clear all of the read-ahead state
* kept for the inode here since the file is now closed.
*/
-int
+void
xfs_inactive(
xfs_inode_t *ip)
{
@@ -1805,16 +1805,14 @@ xfs_inactive(
if (ip->i_d.di_mode == 0 || is_bad_inode(VFS_I(ip))) {
ASSERT(ip->i_df.if_real_bytes == 0);
ASSERT(ip->i_df.if_broot_bytes == 0);
- return VN_INACTIVE_CACHE;
+ return;
}
mp = ip->i_mount;
- error = 0;
-
/* If this is a read-only mount, don't do this (would generate I/O) */
if (mp->m_flags & XFS_MOUNT_RDONLY)
- goto out;
+ return;
if (ip->i_d.di_nlink != 0) {
/*
@@ -1822,12 +1820,10 @@ xfs_inactive(
* cache. Post-eof blocks must be freed, lest we end up with
* broken free space accounting.
*/
- if (xfs_can_free_eofblocks(ip, true)) {
- error = xfs_free_eofblocks(mp, ip, false);
- if (error)
- return VN_INACTIVE_CACHE;
- }
- goto out;
+ if (xfs_can_free_eofblocks(ip, true))
+ xfs_free_eofblocks(mp, ip, false);
+
+ return;
}
if (S_ISREG(ip->i_d.di_mode) &&
@@ -1837,14 +1833,14 @@ xfs_inactive(
error = xfs_qm_dqattach(ip, 0);
if (error)
- return VN_INACTIVE_CACHE;
+ return;
if (S_ISLNK(ip->i_d.di_mode))
error = xfs_inactive_symlink(ip);
else if (truncate)
error = xfs_inactive_truncate(ip);
if (error)
- goto out;
+ return;
/*
* If there are attributes associated with the file then blow them away
@@ -1857,7 +1853,7 @@ xfs_inactive(
error = xfs_attr_inactive(ip);
if (error)
- goto out;
+ return;
}
if (ip->i_afp)
@@ -1870,14 +1866,12 @@ xfs_inactive(
*/
error = xfs_inactive_ifree(ip);
if (error)
- goto out;
+ return;
/*
* Release the dquots held by inode, if any.
*/
xfs_qm_dqdetach(ip);
-out:
- return VN_INACTIVE_CACHE;
}
/*
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 4a91358..cce62ce 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -316,7 +316,7 @@ static inline int xfs_isiflocked(struct xfs_inode *ip)
int xfs_release(struct xfs_inode *ip);
-int xfs_inactive(struct xfs_inode *ip);
+void xfs_inactive(struct xfs_inode *ip);
int xfs_lookup(struct xfs_inode *dp, struct xfs_name *name,
struct xfs_inode **ipp, struct xfs_name *ci_name);
int xfs_create(struct xfs_inode *dp, struct xfs_name *name,
diff --git a/fs/xfs/xfs_vnode.h b/fs/xfs/xfs_vnode.h
index db14d0c..3e8e797 100644
--- a/fs/xfs/xfs_vnode.h
+++ b/fs/xfs/xfs_vnode.h
@@ -25,14 +25,6 @@ struct xfs_inode;
struct attrlist_cursor_kern;
/*
- * Return values for xfs_inactive. A return value of
- * VN_INACTIVE_NOCACHE implies that the file system behavior
- * has disassociated its state and bhv_desc_t from the vnode.
- */
-#define VN_INACTIVE_CACHE 0
-#define VN_INACTIVE_NOCACHE 1
-
-/*
* Flags for read/write calls - same values as IRIX
*/
#define IO_ISDIRECT 0x00004 /* bypass page cache */
--
1.8.1.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 9+ messages in thread