[PATCH] mkfs.xfs: fix protofile name create block reservation

[PATCH] mkfs.xfs: fix protofile name create block reservation

[Eric Sandeen]

A large protofile which creates a large directory and requires
a a dir tree split, can fail:

  mkfs.xfs: directory createname error [28 - No space left on device]

This is because when we've split a block once, we decrement args->total:
(see kernel commit a7444053fb3ebd3d905e3c7a7bd5ea80a54b083a for the
rationale)

       /* account for newly allocated blocks in reserved blocks total */
       args->total -= dp->i_d.di_nblocks - nblks;

but every call into this path from proto file parsing started
reserved / args->total as only "1" as passed tro newdirent() -
so if we allocate a block, args->total hits 0, and then in
xfs_dir2_node_addname():

        /*
         * Add the new leaf entry.
         */
        rval = xfs_dir2_leafn_add(blk->bp, args, blk->index);
        if (rval == 0) {
		...
        } else {
                /*
                 * It didn't work, we need to split the leaf block.
                 */
                if (args->total == 0) {
                        ASSERT(rval == ENOSPC);
                        goto done;
                }
                /*
                 * Split the leaf block and insert the new entry.
                 */

we hit the args->total == 0 special case, and don't do the next
split, and ENOSPC gets returned all the way up, and we fail.

So rather than calling newdirent with a total of "1" in every case,
which doesn't account for possible tree splits, we should call it
with a more appropriate value: XFS_DIRENTER_SPACE_RES(mp, name->len),
which will handle the maximum nr of block allocations that might be
needed during a directory entry insert.

Since the reservation required doesn't depend on entry type,
just push this down a level, into newdirent() itself.

Reported-by: Boris Ranto <branto@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/mkfs/proto.c b/mkfs/proto.c
index f201096..7d96b46 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -306,12 +306,14 @@ newdirent(
 	struct xfs_name	*name,
 	xfs_ino_t	inum,
 	xfs_fsblock_t	*first,
-	xfs_bmap_free_t	*flist,
-	xfs_extlen_t	total)
+	xfs_bmap_free_t	*flist)
 {
 	int	error;
+	int	rsv;
+
+	rsv = XFS_DIRENTER_SPACE_RES(mp, name->len);
 
-	error = libxfs_dir_createname(tp, pip, name, inum, first, flist, total);
+	error = libxfs_dir_createname(tp, pip, name, inum, first, flist, rsv);
 	if (error)
 		fail(_("directory createname error"), error);
 }
@@ -449,7 +451,7 @@ parseproto(
 		if (buf)
 			free(buf);
 		libxfs_trans_ijoin(tp, pip, 0);
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		break;
 
@@ -465,7 +467,7 @@ parseproto(
 
 		libxfs_trans_ijoin(tp, pip, 0);
 
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		libxfs_trans_log_inode(tp, ip, flags);
 
@@ -486,7 +488,7 @@ parseproto(
 			fail(_("Inode allocation failed"), error);
 		}
 		libxfs_trans_ijoin(tp, pip, 0);
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		flags |= XFS_ILOG_DEV;
 		break;
@@ -500,7 +502,7 @@ parseproto(
 		if (error)
 			fail(_("Inode allocation failed"), error);
 		libxfs_trans_ijoin(tp, pip, 0);
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		flags |= XFS_ILOG_DEV;
 		break;
@@ -512,7 +514,7 @@ parseproto(
 		if (error)
 			fail(_("Inode allocation failed"), error);
 		libxfs_trans_ijoin(tp, pip, 0);
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		break;
 	case IF_SYMLINK:
@@ -525,7 +527,7 @@ parseproto(
 			fail(_("Inode allocation failed"), error);
 		flags |= newfile(tp, ip, &flist, &first, 1, 1, buf, len);
 		libxfs_trans_ijoin(tp, pip, 0);
-		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist, 1);
+		newdirent(mp, tp, pip, &xname, ip->i_ino, &first, &flist);
 		libxfs_trans_ihold(tp, pip);
 		break;
 	case IF_DIRECTORY:
@@ -544,7 +546,7 @@ parseproto(
 		} else {
 			libxfs_trans_ijoin(tp, pip, 0);
 			newdirent(mp, tp, pip, &xname, ip->i_ino,
-				  &first, &flist, 1);
+				  &first, &flist);
 			pip->i_d.di_nlink++;
 			libxfs_trans_ihold(tp, pip);
 			libxfs_trans_log_inode(tp, pip, XFS_ILOG_CORE);


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

Re: [PATCH] mkfs.xfs: fix protofile name create block reservation

[Mark Tinguely]

On 08/07/13 01:00, Eric Sandeen wrote:
> A large protofile which creates a large directory and requires
> a a dir tree split, can fail:
>
>    mkfs.xfs: directory createname error [28 - No space left on device]
>
> This is because when we've split a block once, we decrement args->total:
> (see kernel commit a7444053fb3ebd3d905e3c7a7bd5ea80a54b083a for the
> rationale)
>
>         /* account for newly allocated blocks in reserved blocks total */
>         args->total -= dp->i_d.di_nblocks - nblks;
>
> but every call into this path from proto file parsing started
> reserved / args->total as only "1" as passed tro newdirent() -
> so if we allocate a block, args->total hits 0, and then in
> xfs_dir2_node_addname():
>
>          /*
>           * Add the new leaf entry.
>           */
>          rval = xfs_dir2_leafn_add(blk->bp, args, blk->index);
>          if (rval == 0) {
> 		...
>          } else {
>                  /*
>                   * It didn't work, we need to split the leaf block.
>                   */
>                  if (args->total == 0) {
>                          ASSERT(rval == ENOSPC);
>                          goto done;
>                  }
>                  /*
>                   * Split the leaf block and insert the new entry.
>                   */
>
> we hit the args->total == 0 special case, and don't do the next
> split, and ENOSPC gets returned all the way up, and we fail.
>
> So rather than calling newdirent with a total of "1" in every case,
> which doesn't account for possible tree splits, we should call it
> with a more appropriate value: XFS_DIRENTER_SPACE_RES(mp, name->len),
> which will handle the maximum nr of block allocations that might be
> needed during a directory entry insert.
>
> Since the reservation required doesn't depend on entry type,
> just push this down a level, into newdirent() itself.
>
> Reported-by: Boris Ranto<branto@redhat.com>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---

Looks good. This does not harm the current version of the Linux 3.12 crc 
xfsprogs series at all. But we will wait for the weekly community phone 
call before any new commits.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH] mkfs.xfs: fix protofile name create block reservation

[Eric Sandeen]

On 8/7/13 3:19 PM, Mark Tinguely wrote:
> On 08/07/13 01:00, Eric Sandeen wrote:
>> A large protofile which creates a large directory and requires
>> a a dir tree split, can fail:
>>
>>    mkfs.xfs: directory createname error [28 - No space left on device]
>>
>> This is because when we've split a block once, we decrement args->total:
>> (see kernel commit a7444053fb3ebd3d905e3c7a7bd5ea80a54b083a for the
>> rationale)
>>
>>         /* account for newly allocated blocks in reserved blocks total */
>>         args->total -= dp->i_d.di_nblocks - nblks;
>>
>> but every call into this path from proto file parsing started
>> reserved / args->total as only "1" as passed tro newdirent() -
>> so if we allocate a block, args->total hits 0, and then in
>> xfs_dir2_node_addname():
>>
>>          /*
>>           * Add the new leaf entry.
>>           */
>>          rval = xfs_dir2_leafn_add(blk->bp, args, blk->index);
>>          if (rval == 0) {
>>         ...
>>          } else {
>>                  /*
>>                   * It didn't work, we need to split the leaf block.
>>                   */
>>                  if (args->total == 0) {
>>                          ASSERT(rval == ENOSPC);
>>                          goto done;
>>                  }
>>                  /*
>>                   * Split the leaf block and insert the new entry.
>>                   */
>>
>> we hit the args->total == 0 special case, and don't do the next
>> split, and ENOSPC gets returned all the way up, and we fail.
>>
>> So rather than calling newdirent with a total of "1" in every case,
>> which doesn't account for possible tree splits, we should call it
>> with a more appropriate value: XFS_DIRENTER_SPACE_RES(mp, name->len),
>> which will handle the maximum nr of block allocations that might be
>> needed during a directory entry insert.
>>
>> Since the reservation required doesn't depend on entry type,
>> just push this down a level, into newdirent() itself.
>>
>> Reported-by: Boris Ranto<branto@redhat.com>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
> 
> Looks good. This does not harm the current version of the Linux 3.12 crc xfsprogs series at all. But we will wait for the weekly community phone call before any new commits.
> 
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>

Thanks for the review!

I can't imagine that merging this very localized commit would interfere in any way w/ the outstanding CRC stuff; hold off on merge if you really want to, but I don't think anyone would complain if you merged it.

It makes sense to organize pending commits for least disruption, but if there's no disruption involved, I'd say merge sooner rather than later.

Thanks,
-Eric

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

Re: [PATCH] mkfs.xfs: fix protofile name create block reservation

[Mark Tinguely]

On 08/07/13 01:00, Eric Sandeen wrote:
> A large protofile which creates a large directory and requires
> a a dir tree split, can fail:
>
>    mkfs.xfs: directory createname error [28 - No space left on device]
>
> This is because when we've split a block once, we decrement args->total:
> (see kernel commit a7444053fb3ebd3d905e3c7a7bd5ea80a54b083a for the
> rationale)
>
>         /* account for newly allocated blocks in reserved blocks total */
>         args->total -= dp->i_d.di_nblocks - nblks;
>
> but every call into this path from proto file parsing started
> reserved / args->total as only "1" as passed tro newdirent() -
> so if we allocate a block, args->total hits 0, and then in
> xfs_dir2_node_addname():
>
>          /*
>           * Add the new leaf entry.
>           */
>          rval = xfs_dir2_leafn_add(blk->bp, args, blk->index);
>          if (rval == 0) {
> 		...
>          } else {
>                  /*
>                   * It didn't work, we need to split the leaf block.
>                   */
>                  if (args->total == 0) {
>                          ASSERT(rval == ENOSPC);
>                          goto done;
>                  }
>                  /*
>                   * Split the leaf block and insert the new entry.
>                   */
>
> we hit the args->total == 0 special case, and don't do the next
> split, and ENOSPC gets returned all the way up, and we fail.
>
> So rather than calling newdirent with a total of "1" in every case,
> which doesn't account for possible tree splits, we should call it
> with a more appropriate value: XFS_DIRENTER_SPACE_RES(mp, name->len),
> which will handle the maximum nr of block allocations that might be
> needed during a directory entry insert.
>
> Since the reservation required doesn't depend on entry type,
> just push this down a level, into newdirent() itself.
>
> Reported-by: Boris Ranto<branto@redhat.com>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---

This patches and the Coverity xfsprog patches have been commited to the 
master branch:

commit 312592defb8b6051389a56a5c780819b4239dab4
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Thu Aug 1 01:13:10 2013 +0000

     xfsprogs: fix unint var in repair phase6

commit aba29588f81dad52129037c15fe584ec89b36a4f
Author: Eric Sandeen <sandeen@sandeen.net>
Date:   Thu Aug 1 02:25:18 2013 +0000

     xfsprogs: fix agcnts leak in xfs_repair's scan_ags

commit dc93954aa6f13963bcc87fc00ce55a4745dc7b93
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Thu Aug 1 01:33:47 2013 +0000

     xfsprogs:free bp in xlog_find_tail() error path

commit 4623d1041dd34d73aeab4b51002fc9ca6d543415
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Thu Aug 1 01:32:30 2013 +0000

     xfsprogs: free bp in xlog_find_zeroed() error path

commit a134873e2903837bdbeab261b6d0ceee2fd499ba
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Thu Aug 1 01:42:58 2013 +0000

     xfsprogs: fix buffer leak in xlog_print_find_oldest

commit 504dbe46549d462e79565514d86ce69b74b96893
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Thu Aug 1 01:18:54 2013 +0000

     xfsprogs: avoid double-free in xfs_attr_node_addname

commit 1b6bf714a7179942b8523cc966124249b74381da
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Wed Aug 7 06:00:43 2013 +0000

     mkfs.xfs: fix protofile name create block reservation


None collide with the for Linux 3.12 kernel/user sync series.

The kernel patches should make Linux 3.12.

--Mark.


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