[PATCH 0/5] Miscellaneous bug fixes

[PATCH 0/5] Miscellaneous bug fixes

[Dave Chinner]

The following patches fix recently reported bugs and regressions.
All except the log ticket patch should be considered for immediate
inclusion.

[PATCH 1/5] XFS: fix error inversion problems with data flushing

[Dave Chinner]

XFS gets the sign of the error wrong in several places when
gathering the error from generic linux functions. These functions
return negative error values, while the core XFS code returns
positive error values. Hence when XFS inverts the error to be
returned to the VFS, it can incorrectly invert a negative
error and this error will be ignored by the syscall return.

Fix all the problems related to calling filemap_* functions.

Problem initially identified by Nick Piggin in xfs_fsync().

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_fs_subr.c |   23 ++++++++++++++++++++---
 fs/xfs/linux-2.6/xfs_lrw.c     |    2 +-
 fs/xfs/linux-2.6/xfs_super.c   |   13 +++++++++----
 fs/xfs/xfs_vnodeops.c          |    2 +-
 fs/xfs/xfs_vnodeops.h          |    1 +
 5 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_fs_subr.c b/fs/xfs/linux-2.6/xfs_fs_subr.c
index 36caa6d..5aeb777 100644
--- a/fs/xfs/linux-2.6/xfs_fs_subr.c
+++ b/fs/xfs/linux-2.6/xfs_fs_subr.c
@@ -24,6 +24,10 @@ int  fs_noerr(void) { return 0; }
 int  fs_nosys(void) { return ENOSYS; }
 void fs_noval(void) { return; }
 
+/*
+ * note: all filemap functions return negative error codes. These
+ * need to be inverted before returning to the xfs core functions.
+ */
 void
 xfs_tosspages(
 	xfs_inode_t	*ip,
@@ -53,7 +57,7 @@ xfs_flushinval_pages(
 		if (!ret)
 			truncate_inode_pages(mapping, first);
 	}
-	return ret;
+	return -ret;
 }
 
 int
@@ -72,10 +76,23 @@ xfs_flush_pages(
 		xfs_iflags_clear(ip, XFS_ITRUNCATED);
 		ret = filemap_fdatawrite(mapping);
 		if (flags & XFS_B_ASYNC)
-			return ret;
+			return -ret;
 		ret2 = filemap_fdatawait(mapping);
 		if (!ret)
 			ret = ret2;
 	}
-	return ret;
+	return -ret;
+}
+
+int
+xfs_wait_on_pages(
+	xfs_inode_t	*ip,
+	xfs_off_t	first,
+	xfs_off_t	last)
+{
+	struct address_space *mapping = VFS_I(ip)->i_mapping;
+
+	if (mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
+		return -filemap_fdatawait(mapping);
+	return 0;
 }
diff --git a/fs/xfs/linux-2.6/xfs_lrw.c b/fs/xfs/linux-2.6/xfs_lrw.c
index 1957e53..4959c87 100644
--- a/fs/xfs/linux-2.6/xfs_lrw.c
+++ b/fs/xfs/linux-2.6/xfs_lrw.c
@@ -243,7 +243,7 @@ xfs_read(
 
 	if (unlikely(ioflags & IO_ISDIRECT)) {
 		if (inode->i_mapping->nrpages)
-			ret = xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
+			ret = -xfs_flushinval_pages(ip, (*offset & PAGE_CACHE_MASK),
 						    -1, FI_REMAPF_LOCKED);
 		mutex_unlock(&inode->i_mutex);
 		if (ret) {
diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 9dc977d..c5cfc1e 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -1012,21 +1012,26 @@ xfs_fs_write_inode(
 	struct inode		*inode,
 	int			sync)
 {
+	struct xfs_inode	*ip = XFS_I(inode);
 	int			error = 0;
 	int			flags = 0;
 
-	xfs_itrace_entry(XFS_I(inode));
+	xfs_itrace_entry(ip);
 	if (sync) {
-		filemap_fdatawait(inode->i_mapping);
+		error = xfs_wait_on_pages(ip, 0, -1);
+		if (error)
+			goto out_error;
 		flags |= FLUSH_SYNC;
 	}
-	error = xfs_inode_flush(XFS_I(inode), flags);
+	error = xfs_inode_flush(ip, flags);
+
+out_error:
 	/*
 	 * if we failed to write out the inode then mark
 	 * it dirty again so we'll try again later.
 	 */
 	if (error)
-		xfs_mark_inode_dirty_sync(XFS_I(inode));
+		xfs_mark_inode_dirty_sync(ip);
 
 	return -error;
 }
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 2e2fbd9..5809c42 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -713,7 +713,7 @@ xfs_fsync(
 		return XFS_ERROR(EIO);
 
 	/* capture size updates in I/O completion before writing the inode. */
-	error = filemap_fdatawait(VFS_I(ip)->i_mapping);
+	error = xfs_wait_on_pages(ip, 0, -1);
 	if (error)
 		return XFS_ERROR(error);
 
diff --git a/fs/xfs/xfs_vnodeops.h b/fs/xfs/xfs_vnodeops.h
index e932a96..f9cd376 100644
--- a/fs/xfs/xfs_vnodeops.h
+++ b/fs/xfs/xfs_vnodeops.h
@@ -78,5 +78,6 @@ int xfs_flushinval_pages(struct xfs_inode *ip, xfs_off_t first,
 		xfs_off_t last, int fiopt);
 int xfs_flush_pages(struct xfs_inode *ip, xfs_off_t first,
 		xfs_off_t last, uint64_t flags, int fiopt);
+int xfs_wait_on_pages(struct xfs_inode *ip, xfs_off_t first, xfs_off_t last);
 
 #endif /* _XFS_VNODEOPS_H */
-- 
1.5.6.5

Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing

[Christoph Hellwig]

On Fri, Oct 31, 2008 at 12:15:25PM +1100, Dave Chinner wrote:
> XFS gets the sign of the error wrong in several places when
> gathering the error from generic linux functions. These functions
> return negative error values, while the core XFS code returns
> positive error values. Hence when XFS inverts the error to be
> returned to the VFS, it can incorrectly invert a negative
> error and this error will be ignored by the syscall return.
> 
> Fix all the problems related to calling filemap_* functions.
> 
> Problem initially identified by Nick Piggin in xfs_fsync().

But this patch is a little more than just the error code inversions..

Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing

[Dave Chinner]

On Fri, Oct 31, 2008 at 04:18:30PM -0400, Christoph Hellwig wrote:
> On Fri, Oct 31, 2008 at 12:15:25PM +1100, Dave Chinner wrote:
> > XFS gets the sign of the error wrong in several places when
> > gathering the error from generic linux functions. These functions
> > return negative error values, while the core XFS code returns
> > positive error values. Hence when XFS inverts the error to be
> > returned to the VFS, it can incorrectly invert a negative
> > error and this error will be ignored by the syscall return.
> > 
> > Fix all the problems related to calling filemap_* functions.
> > 
> > Problem initially identified by Nick Piggin in xfs_fsync().
> 
> But this patch is a little more than just the error code inversions..

Yes, that's the point of the patch - we get it wrong in many
places and this fixes them so that correctly signed error values
are returned to the higher layers....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/5] XFS: fix error inversion problems with data flushing

[Christoph Hellwig]

On Mon, Nov 03, 2008 at 09:51:33AM +1100, Dave Chinner wrote:
> Yes, that's the point of the patch - we get it wrong in many
> places and this fixes them so that correctly signed error values
> are returned to the higher layers....

Well, you also introduce xfs_wait_on_pages, which should at least be
documented.  And I'd have to say I don't really like adding more of
these wrappers just for the sake of it.

[PATCH 2/5] XFS: Fix double free of log tickets

[Dave Chinner]

When an I/O error occurs during an intermediate commit on a rolling
transaction, xfs_trans_commit() will free the transaction structure
and the related ticket. However, the duplicate transaction that
gets used as the transaction continues still contains a pointer
to the ticket. Hence when the duplicate transaction is cancelled
and freed, we free the ticket a second time.

Add reference counting to the ticket so that we hold an extra
reference to the ticket over the transaction commit. We drop the
extra reference once we have checked that the transaction commit
did not return an error, thus avoiding a double free on commit
error.

Credit to Nick Piggin for tripping over the problem.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_bmap.c     |   10 ++++++++--
 fs/xfs/xfs_inode.c    |   10 ++++++++--
 fs/xfs/xfs_log.c      |   39 +++++++++++++++++++++++++--------------
 fs/xfs/xfs_log.h      |    4 ++++
 fs/xfs/xfs_log_priv.h |    1 +
 fs/xfs/xfs_trans.c    |    9 ++++++++-
 fs/xfs/xfs_utils.c    |    6 ++++++
 fs/xfs/xfs_vnodeops.c |    6 ++++++
 8 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index db28905..c391221 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4292,9 +4292,15 @@ xfs_bmap_finish(
 	 * We have a new transaction, so we should return committed=1,
 	 * even though we're returning an error.
 	 */
-	if (error) {
+	if (error)
 		return error;
-	}
+
+	/*
+	 * transaction commit worked ok so we can drop the extra ticket
+	 * reference that we gained in xfs_trans_dup()
+	 */
+	xfs_log_ticket_put(ntp->t_ticket);
+
 	if ((error = xfs_trans_reserve(ntp, 0, logres, 0, XFS_TRANS_PERM_LOG_RES,
 			logcount)))
 		return error;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index cd52282..b977100 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1782,8 +1782,14 @@ xfs_itruncate_finish(
 		xfs_trans_ijoin(ntp, ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
 		xfs_trans_ihold(ntp, ip);
 
-		if (!error)
-			error = xfs_trans_reserve(ntp, 0,
+		if (error)
+			return error;
+		/*
+		 * transaction commit worked ok so we can drop the extra ticket
+		 * reference that we gained in xfs_trans_dup()
+		 */
+		xfs_log_ticket_put(ntp->t_ticket);
+		error = xfs_trans_reserve(ntp, 0,
 					XFS_ITRUNCATE_LOG_RES(mp), 0,
 					XFS_TRANS_PERM_LOG_RES,
 					XFS_ITRUNCATE_LOG_COUNT);
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index cc1e789..9aecefd 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -100,12 +100,11 @@ STATIC void xlog_ungrant_log_space(xlog_t	 *log,
 
 
 /* local ticket functions */
-STATIC xlog_ticket_t	*xlog_ticket_get(xlog_t *log,
+STATIC xlog_ticket_t	*xlog_ticket_alloc(xlog_t *log,
 					 int	unit_bytes,
 					 int	count,
 					 char	clientid,
 					 uint	flags);
-STATIC void		xlog_ticket_put(xlog_t *log, xlog_ticket_t *ticket);
 
 #if defined(DEBUG)
 STATIC void	xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);
@@ -360,7 +359,7 @@ xfs_log_done(xfs_mount_t	*mp,
 		 */
 		xlog_trace_loggrant(log, ticket, "xfs_log_done: (non-permanent)");
 		xlog_ungrant_log_space(log, ticket);
-		xlog_ticket_put(log, ticket);
+		xfs_log_ticket_put(ticket);
 	} else {
 		xlog_trace_loggrant(log, ticket, "xfs_log_done: (permanent)");
 		xlog_regrant_reserve_log_space(log, ticket);
@@ -514,7 +513,7 @@ xfs_log_reserve(xfs_mount_t	 *mp,
 		retval = xlog_regrant_write_log_space(log, internal_ticket);
 	} else {
 		/* may sleep if need to allocate more tickets */
-		internal_ticket = xlog_ticket_get(log, unit_bytes, cnt,
+		internal_ticket = xlog_ticket_alloc(log, unit_bytes, cnt,
 						  client, flags);
 		if (!internal_ticket)
 			return XFS_ERROR(ENOMEM);
@@ -749,7 +748,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
 		if (tic) {
 			xlog_trace_loggrant(log, tic, "unmount rec");
 			xlog_ungrant_log_space(log, tic);
-			xlog_ticket_put(log, tic);
+			xfs_log_ticket_put(tic);
 		}
 	} else {
 		/*
@@ -3223,22 +3222,33 @@ xlog_state_want_sync(xlog_t *log, xlog_in_core_t *iclog)
  */
 
 /*
- * Free a used ticket.
+ * Free a used ticket when it's refcount falls to zero.
  */
-STATIC void
-xlog_ticket_put(xlog_t		*log,
-		xlog_ticket_t	*ticket)
+void
+xfs_log_ticket_put(
+	xlog_ticket_t	*ticket)
 {
-	sv_destroy(&ticket->t_wait);
-	kmem_zone_free(xfs_log_ticket_zone, ticket);
-}	/* xlog_ticket_put */
+	ASSERT(atomic_read(&ticket->t_ref) > 0);
+	if (atomic_dec_and_test(&ticket->t_ref)) {
+		sv_destroy(&ticket->t_wait);
+		kmem_zone_free(xfs_log_ticket_zone, ticket);
+	}
+}
 
+xlog_ticket_t *
+xfs_log_ticket_get(
+	xlog_ticket_t	*ticket)
+{
+	ASSERT(atomic_read(&ticket->t_ref) > 0);
+	atomic_inc(&ticket->t_ref);
+	return ticket;
+}
 
 /*
  * Allocate and initialise a new log ticket.
  */
 STATIC xlog_ticket_t *
-xlog_ticket_get(xlog_t		*log,
+xlog_ticket_alloc(xlog_t		*log,
 		int		unit_bytes,
 		int		cnt,
 		char		client,
@@ -3309,6 +3319,7 @@ xlog_ticket_get(xlog_t		*log,
 		unit_bytes += 2*BBSIZE;
         }
 
+	atomic_set(&tic->t_ref, 1);
 	tic->t_unit_res		= unit_bytes;
 	tic->t_curr_res		= unit_bytes;
 	tic->t_cnt		= cnt;
@@ -3324,7 +3335,7 @@ xlog_ticket_get(xlog_t		*log,
 	xlog_tic_reset_res(tic);
 
 	return tic;
-}	/* xlog_ticket_get */
+}
 
 
 /******************************************************************************
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index d47b91f..8a3e84e 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -134,6 +134,7 @@ typedef struct xfs_log_callback {
 #ifdef __KERNEL__
 /* Log manager interfaces */
 struct xfs_mount;
+struct xlog_ticket;
 xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
 		       xfs_log_ticket_t ticket,
 		       void		**iclog,
@@ -177,6 +178,9 @@ int	  xfs_log_need_covered(struct xfs_mount *mp);
 
 void	  xlog_iodone(struct xfs_buf *);
 
+struct xlog_ticket * xfs_log_ticket_get(struct xlog_ticket *ticket);
+void	  xfs_log_ticket_put(struct xlog_ticket *ticket);
+
 #endif
 
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index de7ef6c..b39a198 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -245,6 +245,7 @@ typedef struct xlog_ticket {
 	struct xlog_ticket *t_next;	 /*			         :4|8 */
 	struct xlog_ticket *t_prev;	 /*				 :4|8 */
 	xlog_tid_t	   t_tid;	 /* transaction identifier	 : 4  */
+	atomic_t	   t_ref;	 /* ticket reference count       : 4  */
 	int		   t_curr_res;	 /* current reservation in bytes : 4  */
 	int		   t_unit_res;	 /* unit reservation in bytes    : 4  */
 	char		   t_ocnt;	 /* original count		 : 1  */
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index ad137ef..8570b82 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -290,7 +290,7 @@ xfs_trans_dup(
 	ASSERT(tp->t_ticket != NULL);
 
 	ntp->t_flags = XFS_TRANS_PERM_LOG_RES | (tp->t_flags & XFS_TRANS_RESERVE);
-	ntp->t_ticket = tp->t_ticket;
+	ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
 	ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used;
 	tp->t_blk_res = tp->t_blk_res_used;
 	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
@@ -1260,6 +1260,13 @@ xfs_trans_roll(
 	trans = *tpp;
 
 	/*
+	 * transaction commit worked ok so we can drop the extra ticket
+	 * reference that we gained in xfs_trans_dup()
+	 */
+	xfs_log_ticket_put(trans->t_ticket);
+
+
+	/*
 	 * Reserve space in the log for th next transaction.
 	 * This also pushes items in the "AIL", the list of logged items,
 	 * out to disk if they are taking up space at the tail of the log
diff --git a/fs/xfs/xfs_utils.c b/fs/xfs/xfs_utils.c
index 35d4d41..7711449 100644
--- a/fs/xfs/xfs_utils.c
+++ b/fs/xfs/xfs_utils.c
@@ -172,6 +172,12 @@ xfs_dir_ialloc(
 			*ipp = NULL;
 			return code;
 		}
+
+		/*
+		 * 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);
 		code = xfs_trans_reserve(tp, 0, log_res, 0,
 					 XFS_TRANS_PERM_LOG_RES, log_count);
 		/*
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 5809c42..f7eea70 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1029,6 +1029,12 @@ xfs_inactive_symlink_rmt(
 		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).
 	 */
 	if (ip->i_df.if_bytes)
-- 
1.5.6.5

Re: [PATCH 2/5] XFS: Fix double free of log tickets

[Christoph Hellwig]

Looks good to me.  And given that it fixes a regression due to the
conversion of the transaction cache to slab it should get some priority,
maybe even for another 2.6.28 pull.

On Fri, Oct 31, 2008 at 12:15:26PM +1100, Dave Chinner wrote:
> When an I/O error occurs during an intermediate commit on a rolling
> transaction, xfs_trans_commit() will free the transaction structure
> and the related ticket. However, the duplicate transaction that
> gets used as the transaction continues still contains a pointer
> to the ticket. Hence when the duplicate transaction is cancelled
> and freed, we free the ticket a second time.
> 
> Add reference counting to the ticket so that we hold an extra
> reference to the ticket over the transaction commit. We drop the
> extra reference once we have checked that the transaction commit
> did not return an error, thus avoiding a double free on commit
> error.
> 
> Credit to Nick Piggin for tripping over the problem.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_bmap.c     |   10 ++++++++--
>  fs/xfs/xfs_inode.c    |   10 ++++++++--
>  fs/xfs/xfs_log.c      |   39 +++++++++++++++++++++++++--------------
>  fs/xfs/xfs_log.h      |    4 ++++
>  fs/xfs/xfs_log_priv.h |    1 +
>  fs/xfs/xfs_trans.c    |    9 ++++++++-
>  fs/xfs/xfs_utils.c    |    6 ++++++
>  fs/xfs/xfs_vnodeops.c |    6 ++++++
>  8 files changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index db28905..c391221 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -4292,9 +4292,15 @@ xfs_bmap_finish(
>  	 * We have a new transaction, so we should return committed=1,
>  	 * even though we're returning an error.
>  	 */
> -	if (error) {
> +	if (error)
>  		return error;
> -	}
> +
> +	/*
> +	 * transaction commit worked ok so we can drop the extra ticket
> +	 * reference that we gained in xfs_trans_dup()
> +	 */
> +	xfs_log_ticket_put(ntp->t_ticket);
> +
>  	if ((error = xfs_trans_reserve(ntp, 0, logres, 0, XFS_TRANS_PERM_LOG_RES,
>  			logcount)))
>  		return error;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index cd52282..b977100 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1782,8 +1782,14 @@ xfs_itruncate_finish(
>  		xfs_trans_ijoin(ntp, ip, XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL);
>  		xfs_trans_ihold(ntp, ip);
>  
> -		if (!error)
> -			error = xfs_trans_reserve(ntp, 0,
> +		if (error)
> +			return error;
> +		/*
> +		 * transaction commit worked ok so we can drop the extra ticket
> +		 * reference that we gained in xfs_trans_dup()
> +		 */
> +		xfs_log_ticket_put(ntp->t_ticket);
> +		error = xfs_trans_reserve(ntp, 0,
>  					XFS_ITRUNCATE_LOG_RES(mp), 0,
>  					XFS_TRANS_PERM_LOG_RES,
>  					XFS_ITRUNCATE_LOG_COUNT);
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index cc1e789..9aecefd 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -100,12 +100,11 @@ STATIC void xlog_ungrant_log_space(xlog_t	 *log,
>  
>  
>  /* local ticket functions */
> -STATIC xlog_ticket_t	*xlog_ticket_get(xlog_t *log,
> +STATIC xlog_ticket_t	*xlog_ticket_alloc(xlog_t *log,
>  					 int	unit_bytes,
>  					 int	count,
>  					 char	clientid,
>  					 uint	flags);
> -STATIC void		xlog_ticket_put(xlog_t *log, xlog_ticket_t *ticket);
>  
>  #if defined(DEBUG)
>  STATIC void	xlog_verify_dest_ptr(xlog_t *log, __psint_t ptr);
> @@ -360,7 +359,7 @@ xfs_log_done(xfs_mount_t	*mp,
>  		 */
>  		xlog_trace_loggrant(log, ticket, "xfs_log_done: (non-permanent)");
>  		xlog_ungrant_log_space(log, ticket);
> -		xlog_ticket_put(log, ticket);
> +		xfs_log_ticket_put(ticket);
>  	} else {
>  		xlog_trace_loggrant(log, ticket, "xfs_log_done: (permanent)");
>  		xlog_regrant_reserve_log_space(log, ticket);
> @@ -514,7 +513,7 @@ xfs_log_reserve(xfs_mount_t	 *mp,
>  		retval = xlog_regrant_write_log_space(log, internal_ticket);
>  	} else {
>  		/* may sleep if need to allocate more tickets */
> -		internal_ticket = xlog_ticket_get(log, unit_bytes, cnt,
> +		internal_ticket = xlog_ticket_alloc(log, unit_bytes, cnt,
>  						  client, flags);
>  		if (!internal_ticket)
>  			return XFS_ERROR(ENOMEM);
> @@ -749,7 +748,7 @@ xfs_log_unmount_write(xfs_mount_t *mp)
>  		if (tic) {
>  			xlog_trace_loggrant(log, tic, "unmount rec");
>  			xlog_ungrant_log_space(log, tic);
> -			xlog_ticket_put(log, tic);
> +			xfs_log_ticket_put(tic);
>  		}
>  	} else {
>  		/*
> @@ -3223,22 +3222,33 @@ xlog_state_want_sync(xlog_t *log, xlog_in_core_t *iclog)
>   */
>  
>  /*
> - * Free a used ticket.
> + * Free a used ticket when it's refcount falls to zero.
>   */
> -STATIC void
> -xlog_ticket_put(xlog_t		*log,
> -		xlog_ticket_t	*ticket)
> +void
> +xfs_log_ticket_put(
> +	xlog_ticket_t	*ticket)
>  {
> -	sv_destroy(&ticket->t_wait);
> -	kmem_zone_free(xfs_log_ticket_zone, ticket);
> -}	/* xlog_ticket_put */
> +	ASSERT(atomic_read(&ticket->t_ref) > 0);
> +	if (atomic_dec_and_test(&ticket->t_ref)) {
> +		sv_destroy(&ticket->t_wait);
> +		kmem_zone_free(xfs_log_ticket_zone, ticket);
> +	}
> +}
>  
> +xlog_ticket_t *
> +xfs_log_ticket_get(
> +	xlog_ticket_t	*ticket)
> +{
> +	ASSERT(atomic_read(&ticket->t_ref) > 0);
> +	atomic_inc(&ticket->t_ref);
> +	return ticket;
> +}
>  
>  /*
>   * Allocate and initialise a new log ticket.
>   */
>  STATIC xlog_ticket_t *
> -xlog_ticket_get(xlog_t		*log,
> +xlog_ticket_alloc(xlog_t		*log,
>  		int		unit_bytes,
>  		int		cnt,
>  		char		client,
> @@ -3309,6 +3319,7 @@ xlog_ticket_get(xlog_t		*log,
>  		unit_bytes += 2*BBSIZE;
>          }
>  
> +	atomic_set(&tic->t_ref, 1);
>  	tic->t_unit_res		= unit_bytes;
>  	tic->t_curr_res		= unit_bytes;
>  	tic->t_cnt		= cnt;
> @@ -3324,7 +3335,7 @@ xlog_ticket_get(xlog_t		*log,
>  	xlog_tic_reset_res(tic);
>  
>  	return tic;
> -}	/* xlog_ticket_get */
> +}
>  
>  
>  /******************************************************************************
> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
> index d47b91f..8a3e84e 100644
> --- a/fs/xfs/xfs_log.h
> +++ b/fs/xfs/xfs_log.h
> @@ -134,6 +134,7 @@ typedef struct xfs_log_callback {
>  #ifdef __KERNEL__
>  /* Log manager interfaces */
>  struct xfs_mount;
> +struct xlog_ticket;
>  xfs_lsn_t xfs_log_done(struct xfs_mount *mp,
>  		       xfs_log_ticket_t ticket,
>  		       void		**iclog,
> @@ -177,6 +178,9 @@ int	  xfs_log_need_covered(struct xfs_mount *mp);
>  
>  void	  xlog_iodone(struct xfs_buf *);
>  
> +struct xlog_ticket * xfs_log_ticket_get(struct xlog_ticket *ticket);
> +void	  xfs_log_ticket_put(struct xlog_ticket *ticket);
> +
>  #endif
>  
>  
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index de7ef6c..b39a198 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -245,6 +245,7 @@ typedef struct xlog_ticket {
>  	struct xlog_ticket *t_next;	 /*			         :4|8 */
>  	struct xlog_ticket *t_prev;	 /*				 :4|8 */
>  	xlog_tid_t	   t_tid;	 /* transaction identifier	 : 4  */
> +	atomic_t	   t_ref;	 /* ticket reference count       : 4  */
>  	int		   t_curr_res;	 /* current reservation in bytes : 4  */
>  	int		   t_unit_res;	 /* unit reservation in bytes    : 4  */
>  	char		   t_ocnt;	 /* original count		 : 1  */
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index ad137ef..8570b82 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -290,7 +290,7 @@ xfs_trans_dup(
>  	ASSERT(tp->t_ticket != NULL);
>  
>  	ntp->t_flags = XFS_TRANS_PERM_LOG_RES | (tp->t_flags & XFS_TRANS_RESERVE);
> -	ntp->t_ticket = tp->t_ticket;
> +	ntp->t_ticket = xfs_log_ticket_get(tp->t_ticket);
>  	ntp->t_blk_res = tp->t_blk_res - tp->t_blk_res_used;
>  	tp->t_blk_res = tp->t_blk_res_used;
>  	ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
> @@ -1260,6 +1260,13 @@ xfs_trans_roll(
>  	trans = *tpp;
>  
>  	/*
> +	 * transaction commit worked ok so we can drop the extra ticket
> +	 * reference that we gained in xfs_trans_dup()
> +	 */
> +	xfs_log_ticket_put(trans->t_ticket);
> +
> +
> +	/*
>  	 * Reserve space in the log for th next transaction.
>  	 * This also pushes items in the "AIL", the list of logged items,
>  	 * out to disk if they are taking up space at the tail of the log
> diff --git a/fs/xfs/xfs_utils.c b/fs/xfs/xfs_utils.c
> index 35d4d41..7711449 100644
> --- a/fs/xfs/xfs_utils.c
> +++ b/fs/xfs/xfs_utils.c
> @@ -172,6 +172,12 @@ xfs_dir_ialloc(
>  			*ipp = NULL;
>  			return code;
>  		}
> +
> +		/*
> +		 * 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);
>  		code = xfs_trans_reserve(tp, 0, log_res, 0,
>  					 XFS_TRANS_PERM_LOG_RES, log_count);
>  		/*
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 5809c42..f7eea70 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -1029,6 +1029,12 @@ xfs_inactive_symlink_rmt(
>  		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).
>  	 */
>  	if (ip->i_df.if_bytes)
> -- 
> 1.5.6.5
> 
> 
---end quoted text---

[PATCH 3/5] XFS: fix uninitialised variable bug in dquot release.

[Dave Chinner]

gcc on ARM warns about an using an uninitialised variable
in xfs_qm_dqrele_all_inodes(). This is a real bug, but gcc
on x86_64 is not reporting this warning so it went unnoticed.

Fix the bug by bring the inode radix tree walk code up to
date with xfs_sync_inodes_ag().

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/quota/xfs_qm_syscalls.c |   42 ++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/quota/xfs_qm_syscalls.c b/fs/xfs/quota/xfs_qm_syscalls.c
index 9ff28e6..2c57a6e 100644
--- a/fs/xfs/quota/xfs_qm_syscalls.c
+++ b/fs/xfs/quota/xfs_qm_syscalls.c
@@ -1036,9 +1036,6 @@ xfs_qm_dqrele_inodes_ag(
 	int		nr_found;
 
 	do {
-		boolean_t	inode_refed;
-		struct inode	*inode;
-
 		/*
 		 * use a gang lookup to find the next inode in the tree
 		 * as the tree is sparse and a gang lookup walks to find
@@ -1053,27 +1050,37 @@ xfs_qm_dqrele_inodes_ag(
 			break;
 		}
 
-		/* update the index for the next lookup */
+		/*
+		 * Update the index for the next lookup. Catch overflows
+		 * into the next AG range which can occur if we have inodes
+		 * in the last block of the AG and we are currently
+		 * pointing to the last inode.
+		 */
 		first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
+		if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino)) {
+			read_unlock(&pag->pag_ici_lock);
+			break;
+		}
 
-		/* skip quota inodes and those in reclaim */
-		inode = VFS_I(ip);
-		if (!inode || ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
+		/* skip quota inodes */
+		if (ip == XFS_QI_UQIP(mp) || ip == XFS_QI_GQIP(mp)) {
 			ASSERT(ip->i_udquot == NULL);
 			ASSERT(ip->i_gdquot == NULL);
 			read_unlock(&pag->pag_ici_lock);
 			continue;
 		}
-		if (xfs_ilock_nowait(ip, XFS_ILOCK_EXCL) == 0) {
-			inode = igrab(inode);
-			read_unlock(&pag->pag_ici_lock);
-			if (!inode)
-				continue;
-			inode_refed = B_TRUE;
-			xfs_ilock(ip, XFS_ILOCK_EXCL);
-		} else {
+
+		/*
+		 * If we can't get a reference on the inode, it must be
+		 * in reclaim. Leave it for the reclaim code to flush.
+		 */
+		if (!igrab(VFS_I(ip))) {
 			read_unlock(&pag->pag_ici_lock);
+			continue;
 		}
+		read_unlock(&pag->pag_ici_lock);
+
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
 			xfs_qm_dqrele(ip->i_udquot);
 			ip->i_udquot = NULL;
@@ -1083,9 +1090,8 @@ xfs_qm_dqrele_inodes_ag(
 			xfs_qm_dqrele(ip->i_gdquot);
 			ip->i_gdquot = NULL;
 		}
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-		if (inode_refed)
-			IRELE(ip);
+		xfs_iput(ip, XFS_ILOCK_EXCL);
+
 	} while (nr_found);
 }
 
-- 
1.5.6.5

Re: [PATCH 3/5] XFS: fix uninitialised variable bug in dquot release.

[Christoph Hellwig]

On Fri, Oct 31, 2008 at 12:15:27PM +1100, Dave Chinner wrote:
> gcc on ARM warns about an using an uninitialised variable
> in xfs_qm_dqrele_all_inodes(). This is a real bug, but gcc
> on x86_64 is not reporting this warning so it went unnoticed.
> 
> Fix the bug by bring the inode radix tree walk code up to
> date with xfs_sync_inodes_ag().

Looks good.

[PATCH 4/5] XFS: fix spurious uninitialised variable warning in xfs_growfs_rt

[Dave Chinner]

gcc is warning about an uninitialised variable in xfs_growfs_rt().
This is a false positive. Fix it by changing the scope of the
transaction pointer to wholly within the internal loop inside
the function.

While there, preemptively change xfs_growfs_rt_alloc() in the
same way as it has exactly the same structure as xfs_growfs_rt()
but gcc is not warning about it. Yet.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_rtalloc.c |   39 ++++++++++++++++++---------------------
 1 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index e2f68de..f18b9b2 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -85,7 +85,6 @@ xfs_growfs_rt_alloc(
 {
 	xfs_fileoff_t	bno;		/* block number in file */
 	xfs_buf_t	*bp;		/* temporary buffer for zeroing */
-	int		cancelflags;	/* flags for xfs_trans_cancel */
 	int		committed;	/* transaction committed flag */
 	xfs_daddr_t	d;		/* disk block address */
 	int		error;		/* error return value */
@@ -96,15 +95,16 @@ xfs_growfs_rt_alloc(
 	xfs_bmbt_irec_t	map;		/* block map output */
 	int		nmap;		/* number of block maps */
 	int		resblks;	/* space reservation */
-	xfs_trans_t	*tp;		/* transaction pointer */
 
 	/*
 	 * Allocate space to the file, as necessary.
 	 */
 	while (oblocks < nblocks) {
+		int		cancelflags = 0;
+		xfs_trans_t	*tp;
+
 		tp = xfs_trans_alloc(mp, XFS_TRANS_GROWFSRT_ALLOC);
 		resblks = XFS_GROWFSRT_SPACE_RES(mp, nblocks - oblocks);
-		cancelflags = 0;
 		/*
 		 * Reserve space & log for one extent added to the file.
 		 */
@@ -171,7 +171,9 @@ xfs_growfs_rt_alloc(
 				mp->m_bsize, 0);
 			if (bp == NULL) {
 				error = XFS_ERROR(EIO);
-				goto error_cancel;
+error_cancel:
+				xfs_trans_cancel(tp, cancelflags);
+				goto error;
 			}
 			memset(XFS_BUF_PTR(bp), 0, mp->m_sb.sb_blocksize);
 			xfs_trans_log_buf(tp, bp, 0, mp->m_sb.sb_blocksize - 1);
@@ -188,8 +190,6 @@ xfs_growfs_rt_alloc(
 		oblocks = map.br_startoff + map.br_blockcount;
 	}
 	return 0;
-error_cancel:
-	xfs_trans_cancel(tp, cancelflags);
 error:
 	return error;
 }
@@ -1856,7 +1856,6 @@ xfs_growfs_rt(
 {
 	xfs_rtblock_t	bmbno;		/* bitmap block number */
 	xfs_buf_t	*bp;		/* temporary buffer */
-	int		cancelflags;	/* flags for xfs_trans_cancel */
 	int		error;		/* error return value */
 	xfs_inode_t	*ip;		/* bitmap inode, used as lock */
 	xfs_mount_t	*nmp;		/* new (fake) mount structure */
@@ -1872,10 +1871,8 @@ xfs_growfs_rt(
 	xfs_extlen_t	rsumblocks;	/* current number of rt summary blks */
 	xfs_sb_t	*sbp;		/* old superblock */
 	xfs_fsblock_t	sumbno;		/* summary block number */
-	xfs_trans_t	*tp;		/* transaction pointer */
 
 	sbp = &mp->m_sb;
-	cancelflags = 0;
 	/*
 	 * Initial error checking.
 	 */
@@ -1942,6 +1939,9 @@ xfs_growfs_rt(
 		     ((sbp->sb_rextents & ((1 << mp->m_blkbit_log) - 1)) != 0);
 	     bmbno < nrbmblocks;
 	     bmbno++) {
+		xfs_trans_t	*tp;
+		int		cancelflags = 0;
+
 		*nmp = *mp;
 		nsbp = &nmp->m_sb;
 		/*
@@ -1967,16 +1967,15 @@ xfs_growfs_rt(
 		 * Start a transaction, get the log reservation.
 		 */
 		tp = xfs_trans_alloc(mp, XFS_TRANS_GROWFSRT_FREE);
-		cancelflags = 0;
 		if ((error = xfs_trans_reserve(tp, 0,
 				XFS_GROWRTFREE_LOG_RES(nmp), 0, 0, 0)))
-			break;
+			goto error_cancel;
 		/*
 		 * Lock out other callers by grabbing the bitmap inode lock.
 		 */
 		if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0,
 						XFS_ILOCK_EXCL, &ip)))
-			break;
+			goto error_cancel;
 		ASSERT(ip == mp->m_rbmip);
 		/*
 		 * Update the bitmap inode's size.
@@ -1990,7 +1989,7 @@ xfs_growfs_rt(
 		 */
 		if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rsumino, 0,
 						XFS_ILOCK_EXCL, &ip)))
-			break;
+			goto error_cancel;
 		ASSERT(ip == mp->m_rsumip);
 		/*
 		 * Update the summary inode's size.
@@ -2005,7 +2004,7 @@ xfs_growfs_rt(
 		    mp->m_rsumlevels != nmp->m_rsumlevels) {
 			error = xfs_rtcopy_summary(mp, nmp, tp);
 			if (error)
-				break;
+				goto error_cancel;
 		}
 		/*
 		 * Update superblock fields.
@@ -2031,8 +2030,11 @@ xfs_growfs_rt(
 		bp = NULL;
 		error = xfs_rtfree_range(nmp, tp, sbp->sb_rextents,
 			nsbp->sb_rextents - sbp->sb_rextents, &bp, &sumbno);
-		if (error)
+		if (error) {
+error_cancel:
+			xfs_trans_cancel(tp, cancelflags);
 			break;
+		}
 		/*
 		 * Mark more blocks free in the superblock.
 		 */
@@ -2045,15 +2047,10 @@ xfs_growfs_rt(
 		mp->m_rsumsize = nrsumsize;
 
 		error = xfs_trans_commit(tp, 0);
-		if (error) {
-			tp = NULL;
+		if (error)
 			break;
-		}
 	}
 
-	if (error && tp)
-		xfs_trans_cancel(tp, cancelflags);
-
 	/*
 	 * Free the fake mp structure.
 	 */
-- 
1.5.6.5

Re: [PATCH 4/5] XFS: fix spurious uninitialised variable warning in xfs_growfs_rt

[Christoph Hellwig]

On Fri, Oct 31, 2008 at 12:15:28PM +1100, Dave Chinner wrote:
> gcc is warning about an uninitialised variable in xfs_growfs_rt().
> This is a false positive. Fix it by changing the scope of the
> transaction pointer to wholly within the internal loop inside
> the function.
> 
> While there, preemptively change xfs_growfs_rt_alloc() in the
> same way as it has exactly the same structure as xfs_growfs_rt()
> but gcc is not warning about it. Yet.

Looks good.

[PATCH 5/5] XFS: Avoid using inodes that haven't been completely initialised

[Dave Chinner]

The radix tree walks in xfs_sync_inodes_ag and xfs_qm_dqrele_all_inodes()
can find inodes that are still undergoing initialisation. Avoid
them by checking for the the XFS_INEW() flag once we have a reference
on the inode. This flag is cleared once the inode is properly initialised.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/linux-2.6/xfs_iops.c    |    1 -
 fs/xfs/linux-2.6/xfs_sync.c    |    5 +++--
 fs/xfs/quota/xfs_qm_syscalls.c |    6 ++++++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
index b7deff9..f131409 100644
--- a/fs/xfs/linux-2.6/xfs_iops.c
+++ b/fs/xfs/linux-2.6/xfs_iops.c
@@ -772,7 +772,6 @@ xfs_setup_inode(
 	inode->i_ino = ip->i_ino;
 	inode->i_state = I_NEW|I_LOCK;
 	inode_add_to_lists(ip->i_mount->m_super, inode);
-	ASSERT(atomic_read(&inode->i_count) == 1);
 
 	inode->i_mode	= ip->i_d.di_mode;
 	inode->i_nlink	= ip->i_d.di_nlink;
diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
index fb5cca3..d12d31b 100644
--- a/fs/xfs/linux-2.6/xfs_sync.c
+++ b/fs/xfs/linux-2.6/xfs_sync.c
@@ -117,8 +117,9 @@ xfs_sync_inodes_ag(
 		}
 		read_unlock(&pag->pag_ici_lock);
 
-		/* bad inodes are dealt with elsewhere */
-		if (is_bad_inode(inode)) {
+		/* avoid new or bad inodes */
+		if (is_bad_inode(inode) ||
+		    xfs_iflags_test(ip, XFS_INEW)) {
 			IRELE(ip);
 			continue;
 		}
diff --git a/fs/xfs/quota/xfs_qm_syscalls.c b/fs/xfs/quota/xfs_qm_syscalls.c
index 2c57a6e..68139b3 100644
--- a/fs/xfs/quota/xfs_qm_syscalls.c
+++ b/fs/xfs/quota/xfs_qm_syscalls.c
@@ -1080,6 +1080,12 @@ xfs_qm_dqrele_inodes_ag(
 		}
 		read_unlock(&pag->pag_ici_lock);
 
+		/* avoid new inodes though we shouldn't find any here */
+		if (xfs_iflags_test(ip, XFS_INEW)) {
+			IRELE(ip);
+			continue;
+		}
+
 		xfs_ilock(ip, XFS_ILOCK_EXCL);
 		if ((flags & XFS_UQUOTA_ACCT) && ip->i_udquot) {
 			xfs_qm_dqrele(ip->i_udquot);
-- 
1.5.6.5

Re: [PATCH 5/5] XFS: Avoid using inodes that haven't been completely initialised

[Christoph Hellwig]

On Fri, Oct 31, 2008 at 12:15:29PM +1100, Dave Chinner wrote:
> The radix tree walks in xfs_sync_inodes_ag and xfs_qm_dqrele_all_inodes()
> can find inodes that are still undergoing initialisation. Avoid
> them by checking for the the XFS_INEW() flag once we have a reference
> on the inode. This flag is cleared once the inode is properly initialised.

Yeah, that's what I meant with my initial comment that we can grab the
inodes anytime they are on the radix tree.

Looks good.