[PATCH 0/5] xfs: various 2.6.38 candidate bug fixes

[PATCH 0/5] xfs: various 2.6.38 candidate bug fixes

[Dave Chinner]

There's the fix for the specualtive allocation regression that arekm
and hch reported different symptoms of. Some extsize tweaks to
prevent assert failures and btree corruptions. An allocation size
fix to prevent wildly inefficient handling of nullfb allocation
group scanning (demonstrated by low space scanning or single large
preallocation that spans hundreds/thousands of AGs).  Finally,
another a fix for another delayed logging commit path error handling
problem that I noticed by inspection.

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

[PATCH 1/5] xfs: speculative delayed allocation uses rounddown_power_of_2 badly

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

rounddown_power_of_2() returns an undefined result when passed a
value of zero. The specualtive delayed allocation code is doing this
when the inode is zero length. Hence occasionally the preallocation
is much, mcuh large than is necessary (e.g. 8GB for a 270 _byte_
file). Ensure we don't even pass a zero value to this function so
the result of preallocation is always the desired size.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_iomap.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 55582bd..8a0f044 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -337,7 +337,12 @@ xfs_iomap_prealloc_size(
 		int shift = 0;
 		int64_t freesp;
 
-		alloc_blocks = XFS_B_TO_FSB(mp, ip->i_size);
+		/*
+		 * rounddown_pow_of_two() returns an undefined result
+		 * if we pass in alloc_blocks = 0. Hence the "+ 1" to
+		 * ensure we always pass in a non-zero value.
+		 */
+		alloc_blocks = XFS_B_TO_FSB(mp, ip->i_size) + 1;
 		alloc_blocks = XFS_FILEOFF_MIN(MAXEXTLEN,
 					rounddown_pow_of_two(alloc_blocks));
 
-- 
1.7.2.3

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

[PATCH 2/5] xfs: limit extent length for allocation to AG size

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Delayed allocation extents can be larger than AGs, so when trying to
convert a large range we may scan every AG inside
xfs_bmap_alloc_nullfb() trying to find an AG with a size larger than
an AG. We should stop when we find the first AG with a maximum
possible allocation size. This causes excessive CPU usage when there
are lots of AGs.

The same problem occurs when doing preallocation of a range larger
than an AG.

Fix the problem by limiting real allocation lengths to the maximum
that an AG can support. This means if we have empty AGs, we'll stop
the search at the first of them. If there are no empty AGs, we'll
still scan them all, but that is a different problem....

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bmap.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 4111cd3..2ad1daf 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -2430,7 +2430,7 @@ xfs_bmap_btalloc_nullfb(
 		startag = ag = 0;
 
 	pag = xfs_perag_get(mp, ag);
-	while (*blen < ap->alen) {
+	while (*blen < args->maxlen) {
 		if (!pag->pagf_init) {
 			error = xfs_alloc_pagf_init(mp, args->tp, ag,
 						    XFS_ALLOC_FLAG_TRYLOCK);
@@ -2452,7 +2452,7 @@ xfs_bmap_btalloc_nullfb(
 			notinit = 1;
 
 		if (xfs_inode_is_filestream(ap->ip)) {
-			if (*blen >= ap->alen)
+			if (*blen >= args->maxlen)
 				break;
 
 			if (ap->userdata) {
@@ -2498,14 +2498,14 @@ xfs_bmap_btalloc_nullfb(
 	 * If the best seen length is less than the request
 	 * length, use the best as the minimum.
 	 */
-	else if (*blen < ap->alen)
+	else if (*blen < args->maxlen)
 		args->minlen = *blen;
 	/*
 	 * Otherwise we've seen an extent as big as alen,
 	 * use that as the minimum.
 	 */
 	else
-		args->minlen = ap->alen;
+		args->minlen = args->maxlen;
 
 	/*
 	 * set the failure fallback case to look in the selected
@@ -2573,7 +2573,14 @@ xfs_bmap_btalloc(
 	args.tp = ap->tp;
 	args.mp = mp;
 	args.fsbno = ap->rval;
-	args.maxlen = MIN(ap->alen, mp->m_sb.sb_agblocks);
+
+	/*
+	 * The requested extent can be larger than an AG, so trim the block
+	 * count back to the maximum sized extent in an AG. A typical empty AG
+	 * consumes 1 block for headers, 1 block for each btree root (3) and 4
+	 * blocks for the free list.
+	 */
+	args.maxlen = MIN(ap->alen, mp->m_sb.sb_agblocks - 8);
 	args.firstblock = ap->firstblock;
 	blen = 0;
 	if (nullfb) {
@@ -2621,7 +2628,7 @@ xfs_bmap_btalloc(
 			/*
 			 * Adjust for alignment
 			 */
-			if (blen > args.alignment && blen <= ap->alen)
+			if (blen > args.alignment && blen <= args.maxlen)
 				args.minlen = blen - args.alignment;
 			args.minalignslop = 0;
 		} else {
@@ -2640,7 +2647,7 @@ xfs_bmap_btalloc(
 			 * of minlen+alignment+slop doesn't go up
 			 * between the calls.
 			 */
-			if (blen > mp->m_dalign && blen <= ap->alen)
+			if (blen > mp->m_dalign && blen <= args.maxlen)
 				nextminlen = blen - mp->m_dalign;
 			else
 				nextminlen = args.minlen;
-- 
1.7.2.3

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

[PATCH 3/5] xfs: prevent extsize alignment from exceeding maximum extent size

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When doing delayed allocation, if the allocation size is for a
maximally sized extent, extent size alignment can push it over this
limit. This results in an assert failure in xfs_bmbt_set_allf() as
the extent length is too large to find in the extent record.

Fix this by ensuring that we allow for space that extent size
alignment requires (up to 2 * (extsize -1) blocks as we have to
handle both head and tail alignment) when limiting the maximum size
of the extent.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bmap.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 2ad1daf..4901355 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4492,6 +4492,17 @@ xfs_bmapi(
 				/* Figure out the extent size, adjust alen */
 				extsz = xfs_get_extsz_hint(ip);
 				if (extsz) {
+					/*
+					 * make sure we don't exceed a single
+					 * extent length when we align the
+					 * extent by reducing length we are
+					 * going to allocate by the maximum
+					 * amount extent size aligment may
+					 * require.
+					alen = (xfs_extlen_t)XFS_FILBLKS_MIN(
+							len,
+						MAXEXTLEN - (2 * extsz - 1));
+					 */
 					error = xfs_bmap_extsize_align(mp,
 							&got, &prev, extsz,
 							rt, eof,
-- 
1.7.2.3

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

[PATCH 4/5] xfs: limit extsize to size of AGs and/or MAXEXTLEN

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The extent size hint can be set to larger than an AG. This means
that the alignment process can push the range to be allocated
outside the bounds of the AG, resulting in assert failures or
corrupted bmbt records. Similarly, if the extsize is larger than the
maximum extent size supported, the alignment process will produce
extents that are too large to fit into the bmbt records, resulting
in a different type of assert/corruption failure.

Fix this by limiting extsize at the time іt is set firstly to be
less than MAXEXTLEN, then to be a maximum of half the size of the
AGs in the filesystem for non-realtime inodes. Realtime inodes do
not allocate out of AGs, so don't have to be restricted by the size
of AGs.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/linux-2.6/xfs_ioctl.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index b06ede1..f5e2a19 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -985,10 +985,22 @@ xfs_ioctl_setattr(
 
 		/*
 		 * Extent size must be a multiple of the appropriate block
-		 * size, if set at all.
+		 * size, if set at all. It must also be smaller than the
+		 * maximum extent size supported by the filesystem.
+		 *
+		 * Also, for non-realtime files, limit the extent size hint to
+		 * half the size of the AGs in the filesystem so alignment
+		 * doesn't result in extents larger than an AG.
 		 */
 		if (fa->fsx_extsize != 0) {
-			xfs_extlen_t	size;
+			xfs_extlen_t    size;
+			xfs_fsblock_t   extsize_fsb;
+
+			extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
+			if (extsize_fsb > MAXEXTLEN) {
+				code = XFS_ERROR(EINVAL);
+				goto error_return;
+			}
 
 			if (XFS_IS_REALTIME_INODE(ip) ||
 			    ((mask & FSX_XFLAGS) &&
@@ -997,6 +1009,10 @@ xfs_ioctl_setattr(
 				       mp->m_sb.sb_blocklog;
 			} else {
 				size = mp->m_sb.sb_blocksize;
+				if (extsize_fsb > mp->m_sb.sb_agblocks / 2) {
+					code = XFS_ERROR(EINVAL);
+					goto error_return;
+				}
 			}
 
 			if (fa->fsx_extsize % size) {
-- 
1.7.2.3

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

[PATCH 5/5] xfs: handle CIl transaction commit failures correctly

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Failure to commit a transaction into the CIL is not handled
correctly. This currently can only happen when racing with a
shutdown, so it rare. Handle the error similar to a log vector
memory allocation failure, and for both failures clear the PF_TRANS
flag from the task correctly.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_trans.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 50753d3..504a804 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1754,15 +1754,26 @@ xfs_trans_commit_cil(
 	 */
 	log_vector = xfs_trans_alloc_log_vecs(tp);
 	if (!log_vector)
-		return ENOMEM;
+		goto out_enomem;
 
 	error = xfs_log_commit_cil(mp, tp, log_vector, commit_lsn, flags);
-	if (error)
-		return error;
+	if (error) {
+		/*
+		 * We will only get an error if no modifications have been
+		 * made to the items in the transaction. Hence treat it
+		  the same as a memory allocation failure.
+		 */
+		goto out_enomem;
+	}
 
 	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
 	xfs_trans_free(tp);
 	return 0;
+
+out_enomem:
+	/* caller cleans up transaction */
+	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	return ENOMEM;
 }
 
 /*
-- 
1.7.2.3

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

Re: [PATCH 1/5] xfs: speculative delayed allocation uses rounddown_power_of_2 badly

[Christoph Hellwig]

On Wed, Jan 19, 2011 at 03:29:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> rounddown_power_of_2() returns an undefined result when passed a
> value of zero. The specualtive delayed allocation code is doing this
> when the inode is zero length. Hence occasionally the preallocation
> is much, mcuh large than is necessary (e.g. 8GB for a 270 _byte_

	^^^^^^^^^ typo

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 2/5] xfs: limit extent length for allocation to AG size

[Christoph Hellwig]

> +	/*
> +	 * The requested extent can be larger than an AG, so trim the block
> +	 * count back to the maximum sized extent in an AG. A typical empty AG
> +	 * consumes 1 block for headers, 1 block for each btree root (3) and 4
> +	 * blocks for the free list.
> +	 */
> +	args.maxlen = MIN(ap->alen, mp->m_sb.sb_agblocks - 8);

Maybe add a define for the over head blocks to document it more clearly?

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 3/5] xfs: prevent extsize alignment from exceeding maximum extent size

[Christoph Hellwig]

> +					/*
> +					 * make sure we don't exceed a single
> +					 * extent length when we align the
> +					 * extent by reducing length we are
> +					 * going to allocate by the maximum
> +					 * amount extent size aligment may
> +					 * require.
> +					alen = (xfs_extlen_t)XFS_FILBLKS_MIN(
> +							len,
> +						MAXEXTLEN - (2 * extsz - 1));
> +					 */

This essentially just adds a comment, given that the new code is inside
the bracing.  Also the xfs_extlen_t cast seems pointless.  The C type
promotion rules do just fine for going down from a 64bit type to a 32bit
one, as long as the results fit into the latter - which they always do
here.

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

Re: [PATCH 4/5] xfs: limit extsize to size of AGs and/or MAXEXTLEN

[Christoph Hellwig]

On Wed, Jan 19, 2011 at 03:29:59PM +1100, Dave Chinner wrote:
> +		 * Also, for non-realtime files, limit the extent size hint to
> +		 * half the size of the AGs in the filesystem so alignment
> +		 * doesn't result in extents larger than an AG.

What do we do about the last AG, which potentionally is smaller than the
others?

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH 5/5] xfs: handle CIl transaction commit failures correctly

[Christoph Hellwig]

> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 50753d3..504a804 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1754,15 +1754,26 @@ xfs_trans_commit_cil(
>  	 */
>  	log_vector = xfs_trans_alloc_log_vecs(tp);
>  	if (!log_vector)
> -		return ENOMEM;
> +		goto out_enomem;
>  
>  	error = xfs_log_commit_cil(mp, tp, log_vector, commit_lsn, flags);
> -	if (error)
> -		return error;
> +	if (error) {
> +		/*
> +		 * We will only get an error if no modifications have been
> +		 * made to the items in the transaction. Hence treat it
> +		  the same as a memory allocation failure.
> +		 */
> +		goto out_enomem;
> +	}
>  
>  	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
>  	xfs_trans_free(tp);
>  	return 0;
> +
> +out_enomem:
> +	/* caller cleans up transaction */
> +	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
> +	return ENOMEM;

_xfs_trans_commit already restores the process flags for an ENOMEM
return, so the failure from xfs_trans_alloc_log_vecs is already
handled correctly.  If we want to handle the EIO return from
xfs_log_commit_cil the same way it just needs to be turned into an
ENOMEM.  The big questions is if there's any point in having the
shutdown check in xfs_trans_commit_cil - we already do one just before
applying the trans deltas in _xfs_trans_commit, which is handled
correctly and should be sufficient.

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

Re: [PATCH 2/5] xfs: limit extent length for allocation to AG size

[Dave Chinner]

On Mon, Jan 24, 2011 at 04:02:52AM -0500, Christoph Hellwig wrote:
> > +	/*
> > +	 * The requested extent can be larger than an AG, so trim the block
> > +	 * count back to the maximum sized extent in an AG. A typical empty AG
> > +	 * consumes 1 block for headers, 1 block for each btree root (3) and 4
> > +	 * blocks for the free list.
> > +	 */
> > +	args.maxlen = MIN(ap->alen, mp->m_sb.sb_agblocks - 8);
> 
> Maybe add a define for the over head blocks to document it more clearly?
> 

Will do.

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 3/5] xfs: prevent extsize alignment from exceeding maximum extent size

[Dave Chinner]

On Mon, Jan 24, 2011 at 04:04:59AM -0500, Christoph Hellwig wrote:
> > +					/*
> > +					 * make sure we don't exceed a single
> > +					 * extent length when we align the
> > +					 * extent by reducing length we are
> > +					 * going to allocate by the maximum
> > +					 * amount extent size aligment may
> > +					 * require.
> > +					alen = (xfs_extlen_t)XFS_FILBLKS_MIN(
> > +							len,
> > +						MAXEXTLEN - (2 * extsz - 1));
> > +					 */
> 
> This essentially just adds a comment, given that the new code is inside
> the bracing.  Also the xfs_extlen_t cast seems pointless.  The C type
> promotion rules do just fine for going down from a 64bit type to a 32bit
> one, as long as the results fit into the latter - which they always do
> here.

Argh, forgot to refresh the patch after doing some bug hunting.
I'll fix it and retest before I repost it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 4/5] xfs: limit extsize to size of AGs and/or MAXEXTLEN

[Dave Chinner]

On Mon, Jan 24, 2011 at 04:06:56AM -0500, Christoph Hellwig wrote:
> On Wed, Jan 19, 2011 at 03:29:59PM +1100, Dave Chinner wrote:
> > +		 * Also, for non-realtime files, limit the extent size hint to
> > +		 * half the size of the AGs in the filesystem so alignment
> > +		 * doesn't result in extents larger than an AG.
> 
> What do we do about the last AG, which potentionally is smaller than the
> others?

Not sure - I didn't actually consider that case because I forgot
about it.

At first glance it doesn't matter because it won't get
selected because the length of the AG is less than the requested
size unless it is the best match. If it is the best match, we can't
do extsize alignment anyway, so it's the same case as any other
allocation without a freespace extent large enough for alignment.

I'll do a bit more analysis, but I think we're OK here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 5/5] xfs: handle CIl transaction commit failures correctly

[Dave Chinner]

On Mon, Jan 24, 2011 at 04:12:49AM -0500, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > index 50753d3..504a804 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -1754,15 +1754,26 @@ xfs_trans_commit_cil(
> >  	 */
> >  	log_vector = xfs_trans_alloc_log_vecs(tp);
> >  	if (!log_vector)
> > -		return ENOMEM;
> > +		goto out_enomem;
> >  
> >  	error = xfs_log_commit_cil(mp, tp, log_vector, commit_lsn, flags);
> > -	if (error)
> > -		return error;
> > +	if (error) {
> > +		/*
> > +		 * We will only get an error if no modifications have been
> > +		 * made to the items in the transaction. Hence treat it
> > +		  the same as a memory allocation failure.
> > +		 */
> > +		goto out_enomem;
> > +	}
> >  
> >  	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
> >  	xfs_trans_free(tp);
> >  	return 0;
> > +
> > +out_enomem:
> > +	/* caller cleans up transaction */
> > +	current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > +	return ENOMEM;
> 
> _xfs_trans_commit already restores the process flags for an ENOMEM
> return, so the failure from xfs_trans_alloc_log_vecs is already
> handled correctly.  If we want to handle the EIO return from
> xfs_log_commit_cil the same way it just needs to be turned into an
> ENOMEM.  The big questions is if there's any point in having the
> shutdown check in xfs_trans_commit_cil - we already do one just before
> applying the trans deltas in _xfs_trans_commit, which is handled
> correctly and should be sufficient.

True. Removing the shutdown check makes xfs_log_commit_cil() a
function that can have a void return. That's probably a better
solution, so I'll modify it that way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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