[PATCH] e2fsck: fix multiply-claimed block quota accounting when deleting files

[PATCH] e2fsck: fix multiply-claimed block quota accounting when deleting files

[Eric Whitney]

As e2fsck processes each file in pass1, the actual file system quota is
increased by the number of blocks discovered in the file.  This can
include both non-multiply-claimed and multiply-claimed blocks, if the
latter exist.  However, if a file containing multiply-claimed blocks
is then deleted in pass1b, those blocks are not taken into account when
decreasing the actual quota.  In this case, the new quota values written
to the file system by e2fsck overstate the space actually consumed.
And, e2fsck must be run twice on the file system to fully correct
quota.

Fix this by counting multiply-claimed blocks as a debit to quota when
deleting files in pass1b.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 e2fsck/pass1b.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
index b40f026..8744fad 100644
--- a/e2fsck/pass1b.c
+++ b/e2fsck/pass1b.c
@@ -636,11 +636,13 @@ static int delete_file_block(ext2_filsys fs,
 	lc = EXT2FS_B2C(fs, blockcnt);
 	if (ext2fs_test_block_bitmap2(ctx->block_dup_map, *block_nr)) {
 		n = dict_lookup(&clstr_dict, INT_TO_VOIDPTR(c));
-		if (n) {
-			p = (struct dup_cluster *) dnode_get(n);
-			if (lc != pb->cur_cluster)
+		if (n)
+			if (lc != pb->cur_cluster) {
+				p = (struct dup_cluster *) dnode_get(n);
 				decrement_badcount(ctx, *block_nr, p);
-		} else
+				pb->dup_blocks++;
+			}
+		else
 			com_err("delete_file_block", 0,
 			    _("internal error: can't find dup_blk for %llu\n"),
 				*block_nr);
-- 
2.1.4

Re: [PATCH] e2fsck: fix multiply-claimed block quota accounting when deleting files

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]


> On May 10, 2017, at 4:04 PM, Eric Whitney <enwlinux@gmail.com> wrote:
> 
> As e2fsck processes each file in pass1, the actual file system quota is
> increased by the number of blocks discovered in the file.  This can
> include both non-multiply-claimed and multiply-claimed blocks, if the
> latter exist.  However, if a file containing multiply-claimed blocks
> is then deleted in pass1b, those blocks are not taken into account when
> decreasing the actual quota.  In this case, the new quota values written
> to the file system by e2fsck overstate the space actually consumed.
> And, e2fsck must be run twice on the file system to fully correct
> quota.
> 
> Fix this by counting multiply-claimed blocks as a debit to quota when
> deleting files in pass1b.

Nice catch.  It would be good to have an e2fsck test case that checks this.
Also, one minor code style nit (or possibly defect) below.

> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
> e2fsck/pass1b.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
> index b40f026..8744fad 100644
> --- a/e2fsck/pass1b.c
> +++ b/e2fsck/pass1b.c
> @@ -636,11 +636,13 @@ static int delete_file_block(ext2_filsys fs,
> 	lc = EXT2FS_B2C(fs, blockcnt);
> 	if (ext2fs_test_block_bitmap2(ctx->block_dup_map, *block_nr)) {
> 		n = dict_lookup(&clstr_dict, INT_TO_VOIDPTR(c));
> -		if (n) {
> -			p = (struct dup_cluster *) dnode_get(n);
> -			if (lc != pb->cur_cluster)
> 				decrement_badcount(ctx, *block_nr, p);
> -		} else
> +		if (n)
> +			if (lc != pb->cur_cluster) {
> +				p = (struct dup_cluster *) dnode_get(n);
> 				decrement_badcount(ctx, *block_nr, p);
> +				pb->dup_blocks++;
> +			}
> +		else
> 			com_err("delete_file_block", 0,
> 			    _("internal error: can't find dup_blk for %llu\n"),
> 				*block_nr);

This is tricky to know which "if" the "else" is for without the added braces,
and to be honest I don't even know what the C standard says about this, which
is likely why the braces were there in the first place.  I would instead
recommend to add braces around the "else" clause to make it clear.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] e2fsck: fix multiply-claimed block quota accounting when deleting files

[Eric Whitney]

* Andreas Dilger <adilger@dilger.ca>:
> 
> > On May 10, 2017, at 4:04 PM, Eric Whitney <enwlinux@gmail.com> wrote:
> > 
> > As e2fsck processes each file in pass1, the actual file system quota is
> > increased by the number of blocks discovered in the file.  This can
> > include both non-multiply-claimed and multiply-claimed blocks, if the
> > latter exist.  However, if a file containing multiply-claimed blocks
> > is then deleted in pass1b, those blocks are not taken into account when
> > decreasing the actual quota.  In this case, the new quota values written
> > to the file system by e2fsck overstate the space actually consumed.
> > And, e2fsck must be run twice on the file system to fully correct
> > quota.
> > 
> > Fix this by counting multiply-claimed blocks as a debit to quota when
> > deleting files in pass1b.
> 
> Nice catch.  It would be good to have an e2fsck test case that checks this.
> Also, one minor code style nit (or possibly defect) below.

Yeah, there's not much test coverage in this area.  I'll look at it.

> 
> > Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> > ---
> > e2fsck/pass1b.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
> > index b40f026..8744fad 100644
> > --- a/e2fsck/pass1b.c
> > +++ b/e2fsck/pass1b.c
> > @@ -636,11 +636,13 @@ static int delete_file_block(ext2_filsys fs,
> > 	lc = EXT2FS_B2C(fs, blockcnt);
> > 	if (ext2fs_test_block_bitmap2(ctx->block_dup_map, *block_nr)) {
> > 		n = dict_lookup(&clstr_dict, INT_TO_VOIDPTR(c));
> > -		if (n) {
> > -			p = (struct dup_cluster *) dnode_get(n);
> > -			if (lc != pb->cur_cluster)
> > 				decrement_badcount(ctx, *block_nr, p);
> > -		} else
> > +		if (n)
> > +			if (lc != pb->cur_cluster) {
> > +				p = (struct dup_cluster *) dnode_get(n);
> > 				decrement_badcount(ctx, *block_nr, p);
> > +				pb->dup_blocks++;
> > +			}
> > +		else
> > 			com_err("delete_file_block", 0,
> > 			    _("internal error: can't find dup_blk for %llu\n"),
> > 				*block_nr);
> 
> This is tricky to know which "if" the "else" is for without the added braces,
> and to be honest I don't even know what the C standard says about this, which
> is likely why the braces were there in the first place.  I would instead
> recommend to add braces around the "else" clause to make it clear.
> 

Yes, that's a classic dangling else bug - I scrubbed too hard.  Thanks very
much for finding that.  V2 coming along shortly.

Thanks for the review,
Eric