* [PATCH V2] e2fsck: fix multiply-claimed block quota accounting when deleting files
@ 2017-05-11 15:46 Eric Whitney
2017-05-11 16:32 ` Andreas Dilger
0 siblings, 1 reply; 4+ messages in thread
From: Eric Whitney @ 2017-05-11 15:46 UTC (permalink / raw)
To: linux-ext4; +Cc: tytso
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.
[V2] Correct a dangling else bug in the original patch.
Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
e2fsck/pass1b.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
index b40f026..d22cffd 100644
--- a/e2fsck/pass1b.c
+++ b/e2fsck/pass1b.c
@@ -637,9 +637,11 @@ static int delete_file_block(ext2_filsys fs,
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 (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"),
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH V2] e2fsck: fix multiply-claimed block quota accounting when deleting files
2017-05-11 15:46 [PATCH V2] e2fsck: fix multiply-claimed block quota accounting when deleting files Eric Whitney
@ 2017-05-11 16:32 ` Andreas Dilger
2017-05-11 17:27 ` Eric Whitney
0 siblings, 1 reply; 4+ messages in thread
From: Andreas Dilger @ 2017-05-11 16:32 UTC (permalink / raw)
To: Eric Whitney; +Cc: linux-ext4, tytso
[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]
> On May 11, 2017, at 9:46 AM, 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.
>
> [V2] Correct a dangling else bug in the original patch.
>
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> ---
> e2fsck/pass1b.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
> index b40f026..d22cffd 100644
> --- a/e2fsck/pass1b.c
> +++ b/e2fsck/pass1b.c
> @@ -637,9 +637,11 @@ static int delete_file_block(ext2_filsys fs,
> 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 (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"),
My preference would be to have {} around the else clause as well, and I
believe that checkpatch.pl agrees "braces {} should be used on all arms
of this statement". That said, this is a pre-existing condition and is
only code style, while your patch fixes a real bug.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH V2] e2fsck: fix multiply-claimed block quota accounting when deleting files
2017-05-11 16:32 ` Andreas Dilger
@ 2017-05-11 17:27 ` Eric Whitney
2017-05-25 1:36 ` Theodore Ts'o
0 siblings, 1 reply; 4+ messages in thread
From: Eric Whitney @ 2017-05-11 17:27 UTC (permalink / raw)
To: Andreas Dilger; +Cc: Eric Whitney, linux-ext4, tytso
* Andreas Dilger <adilger@dilger.ca>:
>
> > On May 11, 2017, at 9:46 AM, 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.
> >
> > [V2] Correct a dangling else bug in the original patch.
> >
> > Signed-off-by: Eric Whitney <enwlinux@gmail.com>
>
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
>
> > ---
> > e2fsck/pass1b.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/e2fsck/pass1b.c b/e2fsck/pass1b.c
> > index b40f026..d22cffd 100644
> > --- a/e2fsck/pass1b.c
> > +++ b/e2fsck/pass1b.c
> > @@ -637,9 +637,11 @@ static int delete_file_block(ext2_filsys fs,
> > 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 (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"),
>
> My preference would be to have {} around the else clause as well, and I
> believe that checkpatch.pl agrees "braces {} should be used on all arms
> of this statement". That said, this is a pre-existing condition and is
> only code style, while your patch fixes a real bug.
>
Yes, I'd noticed that. The bug I'd inadvertently created came from a quick
attempt to address the coding standard problem by adjusting the previous
clause. I'm going to be modifying this same function again shortly with
more patches (other bugs) - I'll clean up the braces for this else clause
then.
Thanks,
Eric
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-25 1:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-11 15:46 [PATCH V2] e2fsck: fix multiply-claimed block quota accounting when deleting files Eric Whitney
2017-05-11 16:32 ` Andreas Dilger
2017-05-11 17:27 ` Eric Whitney
2017-05-25 1:36 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).