[PATCH] XFS: Account for allocated blocks when expanding directories

[PATCH] XFS: Account for allocated blocks when expanding directories

[Dave Chinner]

When we create a directory, we reserve a number of blocks for
the maximum possible expansion of of the directory due to
various btree splits, freespace allocation, etc. Unfortunately,
each allocation is not reflected in the total number of blocks
still available to the transaction, so the maximal reservation
is used over and over again.

This leads to problems where an allocation group has only
enough blocks for *some* of the allocations required for the
directory modification. After the first N allocations, the
remaining blocks in the allocation group drops below the total
reservation, and subsequent allocations fail because the allocator
will not allow the allocation to proceed if the AG does not have
the enough blocks available for the entire allocation total.

This results in an ENOSPC occurring after an allocation has
already occurred. This results in aborting the directory
operation (leaving the directory in an inconsistent state)
and cancelling a dirty transaction, which results in a filesystem
shutdown.

Avoid the problem by reflecting the number of blocks allocated in
any directory expansion in the total number of blocks available to
the modification in progress. This prevents a directory modification
from being aborted part way through with an ENOSPC.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_da_btree.c |    5 +++++
 fs/xfs/xfs_dir2.c     |    6 ++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 9e561a9..a11a839 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -1566,11 +1566,14 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
 	int nmap, error, w, count, c, got, i, mapi;
 	xfs_trans_t *tp;
 	xfs_mount_t *mp;
+	xfs_drfsbno_t	nblks;
 
 	dp = args->dp;
 	mp = dp->i_mount;
 	w = args->whichfork;
 	tp = args->trans;
+	nblks = dp->i_d.di_nblocks;
+
 	/*
 	 * For new directories adjust the file offset and block count.
 	 */
@@ -1647,6 +1650,8 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
 	}
 	if (mapp != &map)
 		kmem_free(mapp);
+	/* account for newly allocated blocks in reserved blocks total */
+	args->total -= dp->i_d.di_nblocks - nblks;
 	*new_blkno = (xfs_dablk_t)bno;
 	return 0;
 }
diff --git a/fs/xfs/xfs_dir2.c b/fs/xfs/xfs_dir2.c
index 80e0dc5..1afb122 100644
--- a/fs/xfs/xfs_dir2.c
+++ b/fs/xfs/xfs_dir2.c
@@ -525,11 +525,13 @@ xfs_dir2_grow_inode(
 	xfs_mount_t	*mp;
 	int		nmap;		/* number of bmap entries */
 	xfs_trans_t	*tp;
+	xfs_drfsbno_t	nblks;
 
 	xfs_dir2_trace_args_s("grow_inode", args, space);
 	dp = args->dp;
 	tp = args->trans;
 	mp = dp->i_mount;
+	nblks = dp->i_d.di_nblocks;
 	/*
 	 * Set lowest possible block in the space requested.
 	 */
@@ -622,7 +624,11 @@ xfs_dir2_grow_inode(
 	 */
 	if (mapp != &map)
 		kmem_free(mapp);
+
+	/* account for newly allocated blocks in reserved blocks total */
+	args->total -= dp->i_d.di_nblocks - nblks;
 	*dbp = xfs_dir2_da_to_db(mp, (xfs_dablk_t)bno);
+
 	/*
 	 * Update file's size if this is the data space and it grew.
 	 */
-- 
1.5.6

Re: [PATCH] XFS: Account for allocated blocks when expanding directories

[Mark Goodwin]

Hi Dave,

by the looks of it, this is a proposed fix for the bug reported
by Kevin Jamieson (and others in the past) :

"xfs_trans_cancel at line 1138 of file fs/xfs/xfs_trans.c"

Do you (or Kevin or anyone) have a reliable test case to reproduce
this?

Cheers
-- Mark

Dave Chinner wrote:
> When we create a directory, we reserve a number of blocks for
> the maximum possible expansion of of the directory due to
> various btree splits, freespace allocation, etc. Unfortunately,
> each allocation is not reflected in the total number of blocks
> still available to the transaction, so the maximal reservation
> is used over and over again.
> 
> This leads to problems where an allocation group has only
> enough blocks for *some* of the allocations required for the
> directory modification. After the first N allocations, the
> remaining blocks in the allocation group drops below the total
> reservation, and subsequent allocations fail because the allocator
> will not allow the allocation to proceed if the AG does not have
> the enough blocks available for the entire allocation total.
> 
> This results in an ENOSPC occurring after an allocation has
> already occurred. This results in aborting the directory
> operation (leaving the directory in an inconsistent state)
> and cancelling a dirty transaction, which results in a filesystem
> shutdown.
> 
> Avoid the problem by reflecting the number of blocks allocated in
> any directory expansion in the total number of blocks available to
> the modification in progress. This prevents a directory modification
> from being aborted part way through with an ENOSPC.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>
> ---
>  fs/xfs/xfs_da_btree.c |    5 +++++
>  fs/xfs/xfs_dir2.c     |    6 ++++++
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
> index 9e561a9..a11a839 100644
> --- a/fs/xfs/xfs_da_btree.c
> +++ b/fs/xfs/xfs_da_btree.c
> @@ -1566,11 +1566,14 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
>  	int nmap, error, w, count, c, got, i, mapi;
>  	xfs_trans_t *tp;
>  	xfs_mount_t *mp;
> +	xfs_drfsbno_t	nblks;
>  
>  	dp = args->dp;
>  	mp = dp->i_mount;
>  	w = args->whichfork;
>  	tp = args->trans;
> +	nblks = dp->i_d.di_nblocks;
> +
>  	/*
>  	 * For new directories adjust the file offset and block count.
>  	 */
> @@ -1647,6 +1650,8 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
>  	}
>  	if (mapp != &map)
>  		kmem_free(mapp);
> +	/* account for newly allocated blocks in reserved blocks total */
> +	args->total -= dp->i_d.di_nblocks - nblks;
>  	*new_blkno = (xfs_dablk_t)bno;
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_dir2.c b/fs/xfs/xfs_dir2.c
> index 80e0dc5..1afb122 100644
> --- a/fs/xfs/xfs_dir2.c
> +++ b/fs/xfs/xfs_dir2.c
> @@ -525,11 +525,13 @@ xfs_dir2_grow_inode(
>  	xfs_mount_t	*mp;
>  	int		nmap;		/* number of bmap entries */
>  	xfs_trans_t	*tp;
> +	xfs_drfsbno_t	nblks;
>  
>  	xfs_dir2_trace_args_s("grow_inode", args, space);
>  	dp = args->dp;
>  	tp = args->trans;
>  	mp = dp->i_mount;
> +	nblks = dp->i_d.di_nblocks;
>  	/*
>  	 * Set lowest possible block in the space requested.
>  	 */
> @@ -622,7 +624,11 @@ xfs_dir2_grow_inode(
>  	 */
>  	if (mapp != &map)
>  		kmem_free(mapp);
> +
> +	/* account for newly allocated blocks in reserved blocks total */
> +	args->total -= dp->i_d.di_nblocks - nblks;
>  	*dbp = xfs_dir2_da_to_db(mp, (xfs_dablk_t)bno);
> +
>  	/*
>  	 * Update file's size if this is the data space and it grew.
>  	 */

-- 

  Mark Goodwin                                  markgw@sgi.com
  Engineering Manager for XFS and PCP    Phone: +61-3-99631937
  SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Re: [PATCH] XFS: Account for allocated blocks when expanding directories

[Dave Chinner]

On Tue, Sep 30, 2008 at 11:46:08AM +1000, Mark Goodwin wrote:
> Hi Dave,
>
> by the looks of it, this is a proposed fix for the bug reported
> by Kevin Jamieson (and others in the past) :
>
> "xfs_trans_cancel at line 1138 of file fs/xfs/xfs_trans.c"

Yes, it's the main fix needed.

> Do you (or Kevin or anyone) have a reliable test case to reproduce
> this?

Sure - the metadump image that Kevin sent us a pointer to reproduces
it precisely. i.e:

# echo 255 > /proc/sys/fs/xfs/panic_mask
# mount /dev/ubd/5 /mnt/xfs
# cd /mnt/xfs/*/nerd/run1/*5st1\-
# touch aaaaaaaa

<shutdown>

With the patch I posted, it doesn't shut down any more and the
create succeeds. The patch has passed through xfsqa a couple of
times now....

Of the list of things I posted after analysing the problem,
this patch addresses item 1, via the indirect method noted in
item 2, and I've covered all the expansion cases (I think) in the
directory code (item 3).

Item 4 and 5 are less critical given that I don't think they can
cause shutdowns now that the directory reserved block accounting
is correct.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH] XFS: Account for allocated blocks when expanding directories

[Dave Chinner]

When we create a directory, we reserve a number of blocks for
the maximum possible expansion of of the directory due to
various btree splits, freespace allocation, etc. Unfortunately,
each allocation is not reflected in the total number of blocks
still available to the transaction, so the maximal reservation
is used over and over again.

This leads to problems where an allocation group has only
enough blocks for *some* of the allocations required for the
directory modification. After the first N allocations, the
remaining blocks in the allocation group drops below the total
reservation, and subsequent allocations fail because the allocator
will not allow the allocation to proceed if the AG does not have
the enough blocks available for the entire allocation total.

This results in an ENOSPC occurring after an allocation has
already occurred. This results in aborting the directory
operation (leaving the directory in an inconsistent state)
and cancelling a dirty transaction, which results in a filesystem
shutdown.

Avoid the problem by reflecting the number of blocks allocated in
any directory expansion in the total number of blocks available to
the modification in progress. This prevents a directory modification
from being aborted part way through with an ENOSPC.

Signed-off-by: Dave Chinner <david@fromorbit.com>
---
 fs/xfs/xfs_da_btree.c |    5 +++++
 fs/xfs/xfs_dir2.c     |    6 ++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c
index 9e561a9..a11a839 100644
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -1566,11 +1566,14 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
 	int nmap, error, w, count, c, got, i, mapi;
 	xfs_trans_t *tp;
 	xfs_mount_t *mp;
+	xfs_drfsbno_t	nblks;
 
 	dp = args->dp;
 	mp = dp->i_mount;
 	w = args->whichfork;
 	tp = args->trans;
+	nblks = dp->i_d.di_nblocks;
+
 	/*
 	 * For new directories adjust the file offset and block count.
 	 */
@@ -1647,6 +1650,8 @@ xfs_da_grow_inode(xfs_da_args_t *args, xfs_dablk_t *new_blkno)
 	}
 	if (mapp != &map)
 		kmem_free(mapp);
+	/* account for newly allocated blocks in reserved blocks total */
+	args->total -= dp->i_d.di_nblocks - nblks;
 	*new_blkno = (xfs_dablk_t)bno;
 	return 0;
 }
diff --git a/fs/xfs/xfs_dir2.c b/fs/xfs/xfs_dir2.c
index 80e0dc5..1afb122 100644
--- a/fs/xfs/xfs_dir2.c
+++ b/fs/xfs/xfs_dir2.c
@@ -525,11 +525,13 @@ xfs_dir2_grow_inode(
 	xfs_mount_t	*mp;
 	int		nmap;		/* number of bmap entries */
 	xfs_trans_t	*tp;
+	xfs_drfsbno_t	nblks;
 
 	xfs_dir2_trace_args_s("grow_inode", args, space);
 	dp = args->dp;
 	tp = args->trans;
 	mp = dp->i_mount;
+	nblks = dp->i_d.di_nblocks;
 	/*
 	 * Set lowest possible block in the space requested.
 	 */
@@ -622,7 +624,11 @@ xfs_dir2_grow_inode(
 	 */
 	if (mapp != &map)
 		kmem_free(mapp);
+
+	/* account for newly allocated blocks in reserved blocks total */
+	args->total -= dp->i_d.di_nblocks - nblks;
 	*dbp = xfs_dir2_da_to_db(mp, (xfs_dablk_t)bno);
+
 	/*
 	 * Update file's size if this is the data space and it grew.
 	 */
-- 
1.5.6.5