Hi!
I'm sending two fixes to e2fsck that are related with quota handling. The
fixes are the first two patches, the other two are test cases for these
fixes.
The first patch is actually re-send, which Andreas Dilger had already kindly
reviewed and suggested to add a test for it. One (important!) thing I
forgot to mention and to include in that initial email was that there _is_
already a test case for it: it's in the fstests suite, ext4/014. That's how
I found the issue initially. Thus, my test case for e2fsck is nothing but a
filesystem generated with a simplified version of that test.
As for the second issue, it was also found by an fstest, ext4/019, and the
test I'm sending is also based on it.
Cheers,
--
Luis
Luis Henriques (SUSE) (4):
e2fsck: update quota accounting after directory optimization
e2fsck: update quota when deallocating a bad inode
tests: new test to check quota after directory optimization
tests: new test to check quota after a bad inode deallocation
e2fsck/pass2.c | 33 +++++++++++++------
e2fsck/rehash.c | 27 ++++++++++++----
tests/f_quota_deallocate_inode/expect.1 | 18 +++++++++++
tests/f_quota_deallocate_inode/expect.2 | 7 +++++
tests/f_quota_deallocate_inode/image.gz | Bin 0 -> 11594 bytes
tests/f_quota_deallocate_inode/name | 1 +
tests/f_quota_shrinkdir/expect.1 | 40 ++++++++++++++++++++++++
tests/f_quota_shrinkdir/expect.2 | 7 +++++
tests/f_quota_shrinkdir/image.gz | Bin 0 -> 11453 bytes
tests/f_quota_shrinkdir/name | 1 +
10 files changed, 118 insertions(+), 16 deletions(-)
create mode 100644 tests/f_quota_deallocate_inode/expect.1
create mode 100644 tests/f_quota_deallocate_inode/expect.2
create mode 100644 tests/f_quota_deallocate_inode/image.gz
create mode 100644 tests/f_quota_deallocate_inode/name
create mode 100644 tests/f_quota_shrinkdir/expect.1
create mode 100644 tests/f_quota_shrinkdir/expect.2
create mode 100644 tests/f_quota_shrinkdir/image.gz
create mode 100644 tests/f_quota_shrinkdir/name
This new test validates e2fsck by verifying that quota data is updated after a
directory optimization is performed. It mimics fstest ext4/014 by including a
filesystem image where a file is created inside a new directory on the
filesystem root and then root block 0 is wiped:
# debugfs -w -R 'zap -f / 0' f_testnew/image
Signed-off-by: Luis Henriques (SUSE) <[email protected]>
---
tests/f_quota_shrinkdir/expect.1 | 40 +++++++++++++++++++++++++++++++
tests/f_quota_shrinkdir/expect.2 | 7 ++++++
tests/f_quota_shrinkdir/image.gz | Bin 0 -> 11453 bytes
tests/f_quota_shrinkdir/name | 1 +
4 files changed, 48 insertions(+)
create mode 100644 tests/f_quota_shrinkdir/expect.1
create mode 100644 tests/f_quota_shrinkdir/expect.2
create mode 100644 tests/f_quota_shrinkdir/image.gz
create mode 100644 tests/f_quota_shrinkdir/name
diff --git a/tests/f_quota_shrinkdir/expect.1 b/tests/f_quota_shrinkdir/expect.1
new file mode 100644
index 000000000000..812fe44b887d
--- /dev/null
+++ b/tests/f_quota_shrinkdir/expect.1
@@ -0,0 +1,40 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Directory inode 2, block #0, offset 0: directory corrupted
+Salvage? yes
+
+Missing '.' in directory inode 2.
+Fix? yes
+
+Missing '..' in directory inode 2.
+Fix? yes
+
+Pass 3: Checking directory connectivity
+'..' in / (2) is <The NULL inode> (0), should be / (2).
+Fix? yes
+
+Unconnected directory inode 11 (was in /)
+Connect to /lost+found? yes
+
+/lost+found not found. Create? yes
+
+Unconnected directory inode 12 (was in /)
+Connect to /lost+found? yes
+
+Pass 3A: Optimizing directories
+Pass 4: Checking reference counts
+Inode 11 ref count is 3, should be 2. Fix? yes
+
+Inode 12 ref count is 3, should be 2. Fix? yes
+
+Pass 5: Checking group summary information
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
+Update quota info for quota type 0? yes
+
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
+Update quota info for quota type 1? yes
+
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
+Exit status is 1
diff --git a/tests/f_quota_shrinkdir/expect.2 b/tests/f_quota_shrinkdir/expect.2
new file mode 100644
index 000000000000..814f84a54fd6
--- /dev/null
+++ b/tests/f_quota_shrinkdir/expect.2
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
+Exit status is 0
diff --git a/tests/f_quota_shrinkdir/image.gz b/tests/f_quota_shrinkdir/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..753774f6a11f8a60fdbdf2ce205e64a36d753893
GIT binary patch
literal 11453
zcmeI#YgAL|xd335ai~RPTIz*^A*;1VkEMb$7%yPVayrU5$Q=SCgrEpWB1A45hF}tu
z3Y~H*WdsERqH+nj!C*iFAtIxUM=oR8*&De9N+r(Tgb2(g5J+~%uCD$#tH+*yUCWdG
z>s#OZ=l!1VUGMj9V*ZbQH1xa8ahq%s63@lQZkQXly%`x>n?3zvVAIw2zEfTQMdN?n
z4!`*0mQQ{b@-w?%9o~BN+n?8F1biL!!S+)DJwM(tw|V<X$p=3<^*Go5@wLWi_YUoE
z3xE4(j2rmgV)xLJ{#IFkb!)0+$?){-`!><Fq@dvUbAo9uEu+zw^((%DBe7jcRjGge
zg-34T*4e>{g^H5Q;?4)Coh9@)rBCFSH0ST{P0Jki8N799=~_bYkWjf65kHqQeZP|L
z>^wA?r7kp1#xG0hw-ia~&%WO5Mr>PpQ7+_L(#6$jk3GE#&r%Z5l5Y9a8KXTslU>%z
zTJhZ-Zma15J|%drF9Dwu0==O?;QPYE(4TUgIm-P<Cd=%?&3=oEw*q2Fynb1l_kBMp
z9kpk|VuqJNw`IfIKfB6~c8~nkl81>eZ+5UJTAtWWY2z(?{=yge{WrCb4sD1cE8JsX
zIbO9OQ^#LA=xMoGQeh!-E&CO{BMHN~1NgD=dfV)nwXYVV59;Dj%11ZH;krIz#^teB
zzw^AeP1=jRZV?Qb+%FF5)6B(}7{%hd$ScqKB9)_rVz>I<<rS0m!lk*{8T^~*O8RPs
z0mnZ#<6%$K%Zta_n=L+PyQY(EK}iL}7CPDcVaqnrZ_sAXuA*H<+kp@Q`8=*$)^|;>
z$CuJ8@WO2l`GR(~y<tR}F2AMl)<<M%CKdjA`r5?rXS9hbe5eMWSYM%-jH}wqnd{P{
z(9pQ~nkx>kYM1w2@{-SH_v&gWJG%~4WzN~p4EpLQIh!FII*R?}yHblk%3Olfb!5l4
zbG|(~rDg~Dx)c;7fhqT8l?OeQ&ri46{rb?8OPiW;n`Oa6i^B-Mag7p<+s@(HX5!yA
zr;5%$TKS;)(*v}8&DXT9vR(an!Nch03lnu^O?B*Bw6-$wOZIhI3$^J6`vy%x6>Hdq
zv{tBTzoO080T0zBcC50KZf-0XP#^v1#P0F_`tmA|gxSr*_0?X0v2}3d<)eg|V%2~8
zyii`w8^QC|Uca6Zpq8Ax!YJ*?^vtuEJ`P=QrDJeK-&f3Ja;_ZXUFqmqNx5+4vyJ21
z`Wa}=xWjmY)Pw&m(WSXKTtAwr+mwC%**BC9oM>_EiEAVLaH0o$j&WGb!%kb>P9&E&
zEp=L|pJ(E^URoKcE2e!^6$iN};`O0f@CgM+Z*)J7e_0u+o?hiRhxw~4Q->RLLhU+6
zB=oBf?b<DHo!sM*YP6HBYav0N-hU)LDyG5L`2>DBvr>q63M2a6wzS`f$u*!6uW`KU
z_yY@$@9u_mO<Nqj;|!kc<$_yTdRB0c5FXG8?DAHZ)cKslxbxcDI_LAG3nsy<Rowty
zVHl`cnt;aeo6}y$sNHp|hHtuzSKP>!2RUP>y}_RY7YWq7VsjQGIGLK%fC?_4+(<&G
zH~b+mCyX|G<2?(4m1^xvTc3h?g+cPdd2qz|kXO&dU8dxI;c02m)y}gunb{vY&c7Nw
zzMPw^tDn(ci}_lbj65CQXRa9(fL%+(1Lg};6vM0O-Ho}u50gn(Yz(PL2k<dj?kCZ~
zcF1+?4mVkKFN)?aE{9!_d)!!Iq`WB%er^Ax0c4{<@51dUW`pKCESDRj5)JMuI)lw}
zeN>7`Do36HMgZxm*2z$V%m^OCCb>+N`a}Rx;-JYx_h8RW;X*o5?ho!myG$WM=1#By
zePlYzYdaBEBX<XPqIaf`^Nz+r<;WjRDGVCZX|M$O%0%O-x&zDMBPboyoBVj~vPOf(
zh}vSkra*iDUHAk3N5>t4DbgCv5W2HhOGnB9Uvm?mp%$@Gr76)Y<1+<~6sQIn!pco-
zvvjt|0oy~OIkF@>M5ov-72)~tPf4P^@E?J4T2q-Umi2-rE0b?$J*J7MvQU<WCZ)<r
ztO1(19voG?X@439+dO)Eo|+-g0&{^OWmsZ}GrSitDE$*@M(LWyjBZp!Bu3G}GDRr4
z*(HPyA0{bWA{|6|P%n!{?rM{IgL~MWqunl{#j-wd8#cm?)Q1<#_koV+8AUL;qdLf6
zDnt{o@42VRt<|BOU@M!er+KjcXFZ3l!5c~9IQS|MK#0r(Yk_b=ls8xcfZwc~>&|(-
zzjut<X#qS^hHbw866I7;D7&2cg`G2`Jxea_QwH%8K7Mb(m2NM?k59iQ0d_Uc%96r(
zgG&1(%iysF6~-MAcr5)dugEl;zqm<9$yNWUziIkNax~%4x~lz$2AU`6&Lw6aJ-@OR
zO(5;2aPGI1bw3@C`)>-5`Is=VK2+pqbXxSfw=jekSX_tKo${-W#&_hqF$SR=uK^2X
zn)w1Rr&w5Pl(#p#cRKp(UlutzZ7m`YwiZ=J(;q&X9a+X_kQJ^Ux!tc(r}+X~=bltG
zM}@e{%0VAA7F*<=5{AenrD!OIaD9bAaw)74VJ1(hpZo~ujQRqng%KQCJ4i<pv1P8e
zFf<<aLK)aR*IyWHkQy~>s14R*3gA6B5k!<0YdWw6(<i*I2F3oGRp2B~=_VNh<Iz-X
z)$}p1#jPoy?Z*?jiQM5j?1qWOpfbgB_z1FSisOm98CsD6+K#DAeDiI7IHk!I?ZZly
zO?(<fTm!o!h$)s4tZrh$9As1}<I~k*Elfm;u?ADBxz0iph&xCZ$BCln6#+4df4oVJ
zXGk4ol1?y4k#->X8I+>9A`NhbK2xMfgTxR^!IcKGAsVR;k^!s<ntYyy!tVp^%B+dI
zhb38%igkDN!C_Gr%wr8JITP}ErbFWZYiaaOn?#CS#cq!(+e9L4iu)<~z6*62nN}o{
z@3=&aX_m1@ZW_5hy|E9PVfAsZkZY^M{hNfyQ|wDFOCRYkZh$?ITbN|Du9`8-hARR)
zHS?s_<wi1FKH9k~;<44E>TQh?q|R-U9AxV6ql)L9BFUTiBo17$eWwffUmb3IJo#=;
z&jMD(O%etSi5<~Z>>Br+sxd0kT~rP|LtbLP_Y_4%?1ROK2CML_i=xZL!6*jMgi)Nv
zBTyw$;MqDE$q}_fV~8Fr@)QrsGN4Xm0;~3Hn4}xTM%Wg~!D>yHdG}7xh~iXq2K&Nv
zK^Rukw1i$Wr7^;5L_?4cS;DTH&ND)G!d0dqMi5ixgSuiGrKFq6lyktB*no-3>+BBJ
zN?nmG%xpSoZsv!CLB;4kAk=)HPo>D2pay%UtmlWSWm-@HFwAZIFtwZl5>bClXYw&C
zEYwu?!E*<9z4H}aUnGoPxA|As{!Yh}=2y1l^!%UZC^qsXH&G??lfrNUz*W^whI5)c
zkbZ8es&10T5x2u$h{UsVGQ!Z50cRmqT#iaU$s{&9KzT?nR${uylMG6W;eALc5XEa6
zl-nW0z&T$133`oq2;Pb01A&alRKy23!|QO9%E2)7trHX@f+DK{L$P5~gt?0!7zVqe
z6{b+Lk{=9105)b~n7b_j0trP^gSum|i9R6h8Hqj^beB~7gjGVjzeD_m;`0N6rL2?#
z5gDR7)_1h}GU-d!Dy^0(xxso#tD{OaER1#+k|<c8Nf{`pR76cEAh|t~1Q?axiQz^`
zmgck~IWfp6TLbF>t&%yRb_uA|%qmVLM$k1j=t%{T7#xiDV1M9VBunZwz3egWC*-f%
z#CkLkd&NDg4=9%QX^OBF?#JYo>JTqYDY_p6M?0#i{_=Rx1C3DlkUQ&P8R>4ic!C|L
z56pt16dC&9HHfbe5CU3R>4b3GLZ_j3x;p>a;j+inv5==>v81f;Qp-wL?hb;Q^OH*3
zd2sK@d(GeeN9&g8+Snq8f^~szOXZ97%ibiG<HL{HIiXc}-t`+xc=PKN3vox(>`L~V
zb@H2asgCrK^7%^m!OS0iKLf+c<`zR8_ZIa}0??e3hjCO>w&|bcVcoL|SOu&CRspMk
zRlq7>6|f3e1*`&A0jq#jz$#!BunJfOtO8a6tAJI&D)4U?m=cWU*!=WO8YgjP`<{S}
Z|MrZm3H(16$j;kv=2v{eh7H*p{tBVyta$(c
literal 0
HcmV?d00001
diff --git a/tests/f_quota_shrinkdir/name b/tests/f_quota_shrinkdir/name
new file mode 100644
index 000000000000..8772ae5c814b
--- /dev/null
+++ b/tests/f_quota_shrinkdir/name
@@ -0,0 +1 @@
+update quota on directory optimization
If a bad inode is found it will be deallocated. However, if the filesystem has
quota enabled, the quota information isn't being updated accordingly. This
issue was detected by running fstest ext4/019.
This patch fixes the issue by decreasing the inode count from the
quota and, if blocks are also being released, also subtract them as well.
Signed-off-by: Luis Henriques (SUSE) <[email protected]>
---
e2fsck/pass2.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index b91628567a7f..e16b488af643 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -1859,12 +1859,13 @@ static int deallocate_inode_block(ext2_filsys fs,
static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
{
ext2_filsys fs = ctx->fs;
- struct ext2_inode inode;
+ struct ext2_inode_large inode;
struct problem_context pctx;
__u32 count;
struct del_block del_block;
- e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
+ e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
+ sizeof(inode), "deallocate_inode");
clear_problem_context(&pctx);
pctx.ino = ino;
@@ -1874,29 +1875,29 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
e2fsck_read_bitmaps(ctx);
ext2fs_inode_alloc_stats2(fs, ino, -1, LINUX_S_ISDIR(inode.i_mode));
- if (ext2fs_file_acl_block(fs, &inode) &&
+ if (ext2fs_file_acl_block(fs, EXT2_INODE(&inode)) &&
ext2fs_has_feature_xattr(fs->super)) {
pctx.errcode = ext2fs_adjust_ea_refcount3(fs,
- ext2fs_file_acl_block(fs, &inode),
+ ext2fs_file_acl_block(fs, EXT2_INODE(&inode)),
block_buf, -1, &count, ino);
if (pctx.errcode == EXT2_ET_BAD_EA_BLOCK_NUM) {
pctx.errcode = 0;
count = 1;
}
if (pctx.errcode) {
- pctx.blk = ext2fs_file_acl_block(fs, &inode);
+ pctx.blk = ext2fs_file_acl_block(fs, EXT2_INODE(&inode));
fix_problem(ctx, PR_2_ADJ_EA_REFCOUNT, &pctx);
ctx->flags |= E2F_FLAG_ABORT;
return;
}
if (count == 0) {
ext2fs_block_alloc_stats2(fs,
- ext2fs_file_acl_block(fs, &inode), -1);
+ ext2fs_file_acl_block(fs, EXT2_INODE(&inode)), -1);
}
- ext2fs_file_acl_block_set(fs, &inode, 0);
+ ext2fs_file_acl_block_set(fs, EXT2_INODE(&inode), 0);
}
- if (!ext2fs_inode_has_valid_blocks2(fs, &inode))
+ if (!ext2fs_inode_has_valid_blocks2(fs, EXT2_INODE(&inode)))
goto clear_inode;
/* Inline data inodes don't have blocks to iterate */
@@ -1921,10 +1922,22 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
ctx->flags |= E2F_FLAG_ABORT;
return;
}
+
+ if ((ino != quota_type2inum(PRJQUOTA, fs->super)) &&
+ (ino != fs->super->s_orphan_file_inum) &&
+ (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
+ !(inode.i_flags & EXT4_EA_INODE_FL)) {
+ if (del_block.num > 0)
+ quota_data_sub(ctx->qctx, &inode, ino,
+ del_block.num * EXT2_CLUSTER_SIZE(fs->super));
+ quota_data_inodes(ctx->qctx, (struct ext2_inode_large *)&inode,
+ ino, -1);
+ }
+
clear_inode:
/* Inode may have changed by block_iterate, so reread it */
- e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
- e2fsck_clear_inode(ctx, ino, &inode, 0, "deallocate_inode");
+ e2fsck_read_inode(ctx, ino, EXT2_INODE(&inode), "deallocate_inode");
+ e2fsck_clear_inode(ctx, ino, EXT2_INODE(&inode), 0, "deallocate_inode");
}
/*
In "Pass 3A: Optimizing directories", a directory may have it's size reduced.
If that happens and quota is enabled in the filesystem, the quota information
will be incorrect because it doesn't take the rehash into account. This issue
was detected by running fstest ext4/014.
This patch simply updates the quota data accordingly, after the directory is
written and it's size has been updated.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218626
Signed-off-by: Luis Henriques (SUSE) <[email protected]>
---
e2fsck/rehash.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
index c1da7d52724e..4847d172e5fe 100644
--- a/e2fsck/rehash.c
+++ b/e2fsck/rehash.c
@@ -987,14 +987,18 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
{
ext2_filsys fs = ctx->fs;
errcode_t retval;
- struct ext2_inode inode;
+ struct ext2_inode_large inode;
char *dir_buf = 0;
struct fill_dir_struct fd = { NULL, NULL, 0, 0, 0, NULL,
0, 0, 0, 0, 0, 0 };
struct out_dir outdir = { 0, 0, 0, 0 };
- struct name_cmp_ctx name_cmp_ctx = {0, NULL};
+ struct name_cmp_ctx name_cmp_ctx = {0, NULL};
+ __u64 osize;
- e2fsck_read_inode(ctx, ino, &inode, "rehash_dir");
+ e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
+ sizeof(inode), "rehash_dir");
+
+ osize = EXT2_I_SIZE(&inode);
if (ext2fs_has_feature_inline_data(fs->super) &&
(inode.i_flags & EXT4_INLINE_DATA_FL))
@@ -1013,7 +1017,7 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
fd.ino = ino;
fd.ctx = ctx;
fd.buf = dir_buf;
- fd.inode = &inode;
+ fd.inode = EXT2_INODE(&inode);
fd.dir = ino;
if (!ext2fs_has_feature_dir_index(fs->super) ||
(inode.i_size / fs->blocksize) < 2)
@@ -1092,14 +1096,25 @@ resort:
goto errout;
}
- retval = write_directory(ctx, fs, &outdir, ino, &inode, fd.compress);
+ retval = write_directory(ctx, fs, &outdir, ino, EXT2_INODE(&inode),
+ fd.compress);
if (retval)
goto errout;
+ if ((osize > EXT2_I_SIZE(&inode)) &&
+ (ino != quota_type2inum(PRJQUOTA, fs->super)) &&
+ (ino != fs->super->s_orphan_file_inum) &&
+ (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
+ !(inode.i_flags & EXT4_EA_INODE_FL)) {
+ quota_data_sub(ctx->qctx, &inode,
+ ino, osize - EXT2_I_SIZE(&inode));
+ }
+
if (ctx->options & E2F_OPT_CONVERT_BMAP)
retval = e2fsck_rebuild_extents_later(ctx, ino);
else
- retval = e2fsck_check_rebuild_extents(ctx, ino, &inode, pctx);
+ retval = e2fsck_check_rebuild_extents(ctx, ino,
+ EXT2_INODE(&inode), pctx);
errout:
ext2fs_free_mem(&dir_buf);
ext2fs_free_mem(&fd.harray);
This new test validates e2fsck by verifying that quota is updated after a bad
inode is deallocated. It mimics fstest ext4/019 by including a filesystem image
where a symbolic link was created to an existing file, using a long symlink
name. This symbolic link was then wiped with:
# debugfs -w -R 'zap -f /testlink 0' f_testnew/image
Signed-off-by: Luis Henriques (SUSE) <[email protected]>
---
tests/f_quota_deallocate_inode/expect.1 | 18 ++++++++++++++++++
tests/f_quota_deallocate_inode/expect.2 | 7 +++++++
tests/f_quota_deallocate_inode/image.gz | Bin 0 -> 11594 bytes
tests/f_quota_deallocate_inode/name | 1 +
4 files changed, 26 insertions(+)
create mode 100644 tests/f_quota_deallocate_inode/expect.1
create mode 100644 tests/f_quota_deallocate_inode/expect.2
create mode 100644 tests/f_quota_deallocate_inode/image.gz
create mode 100644 tests/f_quota_deallocate_inode/name
diff --git a/tests/f_quota_deallocate_inode/expect.1 b/tests/f_quota_deallocate_inode/expect.1
new file mode 100644
index 000000000000..2b2f128dbb57
--- /dev/null
+++ b/tests/f_quota_deallocate_inode/expect.1
@@ -0,0 +1,18 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Symlink /testlink (inode #14) is invalid.
+Clear? yes
+
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (15360, 4) != expected (16384, 5)
+Update quota info for quota type 0? yes
+
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (15360, 4) != expected (16384, 5)
+Update quota info for quota type 1? yes
+
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 13/256 files (15.4% non-contiguous), 1157/8192 blocks
+Exit status is 1
diff --git a/tests/f_quota_deallocate_inode/expect.2 b/tests/f_quota_deallocate_inode/expect.2
new file mode 100644
index 000000000000..802317949959
--- /dev/null
+++ b/tests/f_quota_deallocate_inode/expect.2
@@ -0,0 +1,7 @@
+Pass 1: Checking inodes, blocks, and sizes
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+test_filesys: 13/256 files (15.4% non-contiguous), 1157/8192 blocks
+Exit status is 0
diff --git a/tests/f_quota_deallocate_inode/image.gz b/tests/f_quota_deallocate_inode/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..798a72c3a0da0323fc55c5e034f778e967394c81
GIT binary patch
literal 11594
zcmeI%eNYqUx&UyiRgMQsPLKCM5rNy&@i@JTaO(gyn54Bwu%aS@KoXLGC`cj%3rjQ*
zg2hU`eAU`Q6+=`M67t1|OCTYMNYyF=B1u;HiWUXBD+!3pM<Bb`+nL^fZpZWAnK|C<
zpU=B9@B9At*>|63*0<;ne|Yru^5mt<QqvMs4lQ{;=yG8#(4PN9*SUP@$xXViE`Pne
zK0NZ1cQ*uWjI00T&$OjGws9}5lt>bmzrFo={>8rrML%Ee8uX`}&l>)HyIhiM`?l!8
zzr9U)`^DDT9B%VE?Pm3~_)+VS*Q)$^^N!t~t4cW<+s=aa??*fw9p6o9CU1POevfa^
zM=hi06`Q<TXR<xAFb!_H|JRX}p!APutBE?Jrw3Os_l&O0`{~@6H}6N!s8ZdNjyUej
zy&-dDYv=QUpv|(O@oztewg~QC=u5T=Zj8EA9W%S9i1~xR6qUSe`CAz)_(IuGWp~|^
zf``>Javj^&Upu34uyNu=H+g|lU0?oQys&q7E7rKEE_>OXx~;@y{7=~fQSTZsSrM3<
zr^-4+iJ<IQ^7?C(0u8or#C*xvyOeD$;Ani)9T6vY36?Xe81q=W<*2=FwDzsZ2VM4{
zARgwwmTL`6D(IaWSXCQ^i><<)qRj$Fbz#@SmO-Uge!)WXVnW7~l2PHofL2?0u_awq
z-iv{}iKqp0UJ@>vHz-|g7pH}vO!Iem$GMm;5a%`p;P=Kt@;k<Gds0%c4<5Fna`+a$
zuwDK0JWlb%X`Ng)=27*sXJCTa#Lk|G%yGN@W50GrD&QFUg&)TlPXPCS@)ly}45E-h
zoQl`X;Gu<;H-$gMbZlr=d9!c#OS6@pflb*`21h+m@6hVR`1A~(-sbSt4P%x|>O{Js
zYvyd;lx-AW9N|?0X*pKI!X#GMH^9Lx59Wf8XY?&ls-Z2nf*P+Y3}vzia_Up}E+HcX
z-_2Ezh}RQVRP1V*pQk(zT3jrnPD>`z?_hZS6Gu;u-|O99KbjF1Ks)R5K{bMnyc{3b
zh^SZkcwQT~5|bcmlZ_;RNZIFzZRr7V2kNhn%Sp{5&4{(2{eTF6dGd4D?y4ZpOtClb
z(W3_^Ln^kX*JG@}M5~=}Z~d)}OY5;^X5n=^Ar5~Fp(Hq5<aj>n`)lxV_kkO8e+p=D
zmn`S(^NV&Rj0!>%0^+g)W;auc2&cr$)~qU*UuFN&KH0Rbp#3B8h+k|c^hfZ3B6<V#
zA0S&16U7FKe6ZT?p~D3bo=HeJGJNOE)RV`d<;0Pn6$C?Wr-`s-x^1NN<D|O{BWm4;
z;c=Rb+Bm!rnVcLEa<HPxlJ2x4hN)lLM=FMray7T+o)wmgS)5!VkgJ)Q%UaC4<>qAp
zmtcj;M^BTLF4ji#dzNuR@WTaV*0Y2!3r+-BJX_Bp>oM7eAn^ICEnXMhX{|)l9PN+4
zeD$)5p+Qcl%HeI@+Q&Y2a|ZluA}+)<Sw`-boCCurXj+HMEalmXG5k2LE(<T5wuBG6
zYAwgRD)HX-wy1*kOX??wrh*0{>^BXS_;i^i%?wVo;?|A@-#xGLK*$Ry!YgL!`NGuh
zNdYMsofQ_^(AI-DuAF&dnGxwnic{VHG6)PE!?{=h7q@-sHG!8fxHuQa`y7N>y!)IU
z>+Qfi77UrEY)|8`ni*l(Eqr((J_jtGQRz71e)Wvkg6$p-Fpjy1@MY(r&*1As$*8fN
zcVd2a7Ei>`@`5oX6dZ_^FvkLCep;%=$V&43l{56L#cCx9>wTFWf*pM^33PY$;EC!)
zIVHKPx>>t!08i{#5Uc%{;1;(v)@7!Q;PG$P=uhzGMh82Po50__sbQvY_$u1ChWsgu
zsglIHiAMq#1Fp+p6MCMXrc<8c29TAds@3p2zyVP-S021y_T+IFa5rODo!UGAPoSmz
z6rF63q7Qz_=ZPXy%sH?Q)e~jItQ5!vE<i8YGXw`)^rkQ*37sOU$&q4t#5=R8@MiRp
zJzk*K?`6rS;c)bseWw8UIL`#Z2wGxK7I0epDQjbq0U%OPtEXFJHi!ThqSf}Jjw^XI
zimV733*4HK`?;erZ@)olS;bNtxk#g?O)1%c@PPdeJ+@aUr<lZue@8<LMia^mgxiLx
zx6-eKa$cRHKhiF`%RdMA`o$L+pVF?m%WiT`(;Ca`t|IB+E<bvw=>@n=5x2o~7u>0!
z$xPqTbY@8)N1@3Xsx~Wg%tSz?;SJSV>4=#R<_)Q=EP}+vTmZMD-|!Eo?e#K;f=5Ym
zA7-Vo2BeZM`%o*PYIYGni&W!7FO%JdR<k?#2T0PY_y}pOIR?GP=aZ_dsC1|aeuQ4&
zr>8~r$i^U7_$x3iEjEfn+pU`56!te{*NHfxU2+B+>Q^wMwUGQgBga?;#!fI6jCznY
z!E!O4_+Z3K9klKCJO1=k>vZzdQ(lq}GyN~91#3OyGrM9cO5$_0h@L5$wYhsznx+!0
zV=D`Lq?L1n_fdIp&S*N)HF#(y&8sU$+q>?pGSmLumfEhtg_B)`ty6NpZZg`^=ix;j
zThAJsyFFR4sl&VM*ALfPy;bpR2A}fHe<+;e*SoDP6+`s6@WF%u^0Q6&!s+8;ZxNhU
z&D)76$2u!iS{UkbVCjp~!V$4ed#EO2>x&zp7#;2_smd737NNo$!bVKBIs0a6xzFoQ
z(KmVY9&5CC)e4s~w-trE>Nh6jWo97w`@muEm_n!s`I^{RT32BVM-Bm5x;AE{N}dEi
z<nPyMm{BSPWI~V@qJEDg*mxS)r)wCFUJq3w-|@qB;BX9A#WRK=41g_)sfDyiF|lbl
z%BGk#QBjPjCGYi>6HJB32f!Y|HFAuvDic|a-mymuG~_6;qQ^9bxS}_SDt$CdsWQeQ
zi*}kor;oJAxu)enqCmT;&SvBxIjF<_siQtGnxYgNBajr}sG}_}vcl+(#G;6OH-lzS
zbiyw*mz8phX+OGP|IE>VMGKV%qX~uVbcaqU{d5G|QDQ#1Ms^zf#4qkF`x33iU3v#R
z>=#o6{TV#yw?7;jVxOZmm&=}UzM?h6)P2wSo(9IqJ_PxSNRF8X%xc0Ej9EDVGXa|>
zVn}UPplpYRHDvJFOOU&m3Xu2F`~4S#<R$D8ejKSSNV=QT#Q!^~F-R3=a!1GcyGYGG
zv1O|D#&YyW{ytJ`)xHRY0!~F=@<T~Y&B_#06S4&z?60Yc?NNCcT{#2&${O>v^UMLr
z4NRJd4uOi#Q?*7VNY_cSRn$Q3jD*Bd{Z`c&+$778i|0497ni-s`TGy|mj3*k-uB1$
ziE=lU2lOs*(0jkXw8Gp8kMob~E;FgOjSXlWAJEk>=_(ln9R+eld%2R;$Q6E)PP#`K
z41I`}^Eo=jFw-XCnYFNysMw?8L7qUeu6CGilQqE(AX{*e9Oqj{fWqJl_QL`>nK@=&
z51&P)_AEgSnJ$*~Kpt=rz!XUJ@hqvzJdc8QzMxu9wbZ%7H|*JrXp7Wlo`o0C0(+uE
zk{3^rip@Rn&-UYv%bTPuvkHdLO3k%AdWGp3u$@6Q$Yk&=T4+ylNU?aKREDTfwLOOs
zRj{(4{mswlD3^!;?5%(J+~4FIg0sg@wm+^XwlU+wjAwvo-4!OyUsjDgL;s6^$UEw`
z`G4ywChN9o3fQK*v`2mxE<tOEt;6xPMmnrOoB27q>S3y_E(*Cu)D1IIjOWlQK3Ave
zuA7A(p(pGK0tq?ZSDFb|qYd_BBDz=-VXlSy&~x@90wtNnlK7jCqgU-;2(IdBW6*CE
zm28v5^>y_WTBNy>M>W*3ATIpCp2mo&fc^p;awwH`7RVPaLu>7sj;l(g!EAwT!0vZ$
zttdFNMzJz5W;N#ot^RMOZD5AqzO$TOn%rHrj(vw#SFY5Wez%E0Y1L*q2wQ<Nn$}81
zj5>H25No&!I%2W|291z((I<{j=klroz65@n7UyNUh1T<rrR}RUC!=5U52i88>ipnd
z^auVClG2A2A@M^_qgVJ}kgitI=%zmS7qpn4oW|&>OMx=sR&X~-RmJj`c$k|(#%@Jw
zpzi;4jr_>pTF+9?o;Nu;e|K;BA^LT(A+wjy)MN(5kN2N)f6tJ9`Q|cS>gtV4f8dr-
z-wv=emJx~v`;Q)o>U6WpL;YTb6}m4+@BqUXi@u|*QoLialT3a*N4Y0lix<vAPdQ$m
z3|{WWAGW^d%}{SH%C>*I@@!~wL}g%(y>xR8b>Hu<!x{W12wZpTOTPCIxXaX9$w}~y
z@)iGtIh|8D1)Ks-0jGddz$xGqa0)mDoB~b(r+`zyDc}@v3OEIv0!{&^fK%XKNMKSp
k(f<Db{OruXh^uso|3WKwF7^L|z}z2?A5?k2O3x+#4eAK~X#fBK
literal 0
HcmV?d00001
diff --git a/tests/f_quota_deallocate_inode/name b/tests/f_quota_deallocate_inode/name
new file mode 100644
index 000000000000..396887c16f84
--- /dev/null
+++ b/tests/f_quota_deallocate_inode/name
@@ -0,0 +1 @@
+update quota when deallocating bad inode
On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <[email protected]> wrote:
>
> In "Pass 3A: Optimizing directories", a directory may have it's size reduced.
> If that happens and quota is enabled in the filesystem, the quota information
> will be incorrect because it doesn't take the rehash into account. This issue
> was detected by running fstest ext4/014.
>
> This patch simply updates the quota data accordingly, after the directory is
> written and it's size has been updated.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218626
> Signed-off-by: Luis Henriques (SUSE) <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> e2fsck/rehash.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c
> index c1da7d52724e..4847d172e5fe 100644
> --- a/e2fsck/rehash.c
> +++ b/e2fsck/rehash.c
> @@ -987,14 +987,18 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
> {
> ext2_filsys fs = ctx->fs;
> errcode_t retval;
> - struct ext2_inode inode;
> + struct ext2_inode_large inode;
> char *dir_buf = 0;
> struct fill_dir_struct fd = { NULL, NULL, 0, 0, 0, NULL,
> 0, 0, 0, 0, 0, 0 };
> struct out_dir outdir = { 0, 0, 0, 0 };
> - struct name_cmp_ctx name_cmp_ctx = {0, NULL};
> + struct name_cmp_ctx name_cmp_ctx = {0, NULL};
> + __u64 osize;
>
> - e2fsck_read_inode(ctx, ino, &inode, "rehash_dir");
> + e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
> + sizeof(inode), "rehash_dir");
> +
> + osize = EXT2_I_SIZE(&inode);
>
> if (ext2fs_has_feature_inline_data(fs->super) &&
> (inode.i_flags & EXT4_INLINE_DATA_FL))
> @@ -1013,7 +1017,7 @@ errcode_t e2fsck_rehash_dir(e2fsck_t ctx, ext2_ino_t ino,
> fd.ino = ino;
> fd.ctx = ctx;
> fd.buf = dir_buf;
> - fd.inode = &inode;
> + fd.inode = EXT2_INODE(&inode);
> fd.dir = ino;
> if (!ext2fs_has_feature_dir_index(fs->super) ||
> (inode.i_size / fs->blocksize) < 2)
> @@ -1092,14 +1096,25 @@ resort:
> goto errout;
> }
>
> - retval = write_directory(ctx, fs, &outdir, ino, &inode, fd.compress);
> + retval = write_directory(ctx, fs, &outdir, ino, EXT2_INODE(&inode),
> + fd.compress);
> if (retval)
> goto errout;
>
> + if ((osize > EXT2_I_SIZE(&inode)) &&
> + (ino != quota_type2inum(PRJQUOTA, fs->super)) &&
> + (ino != fs->super->s_orphan_file_inum) &&
> + (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
> + !(inode.i_flags & EXT4_EA_INODE_FL)) {
> + quota_data_sub(ctx->qctx, &inode,
> + ino, osize - EXT2_I_SIZE(&inode));
> + }
> +
> if (ctx->options & E2F_OPT_CONVERT_BMAP)
> retval = e2fsck_rebuild_extents_later(ctx, ino);
> else
> - retval = e2fsck_check_rebuild_extents(ctx, ino, &inode, pctx);
> + retval = e2fsck_check_rebuild_extents(ctx, ino,
> + EXT2_INODE(&inode), pctx);
> errout:
> ext2fs_free_mem(&dir_buf);
> ext2fs_free_mem(&fd.harray);
Cheers, Andreas
On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <[email protected]> wrote:
>
> If a bad inode is found it will be deallocated. However, if the
> filesystem has quota enabled, the quota information isn't being updated
> accordingly. This issue was detected by running fstest ext4/019.
>
> This patch fixes the issue by decreasing the inode count from the quota
> and, if blocks are also being released, also subtract them as well.
I was wondering about the safety of this patch, since "bad inode"
usually means corrupted inode with random garbage, and continuing to
process it is as likely to introduce problems as it is to fix them.
The only caller of deallocate_inode() is e2fsck_process_bad_inode()
when the file type is bad or not matching the inode content. This
appears it would mostly affect only the inode quota, though in some
rare cases it might affect the block quota (xattr or long symlink).
Looking at the history of deallocate_inode(), it appears risky that
it processes blocks from a corrupt inode at all. The saving grace
is that it should only "undo" the blocks marked in-use by pass1 so
that a second e2fsck run is not needed to fix up the bitmaps and
counters. It looks like the same is true with the quota accounting,
that deallocate_inode() is only updating the in-memory inode/block
counters for the UID/GID/PRJID but not messing with any on-disk data
until on-disk quotas are updated in pass5.
It wouldn't be terrible to update the comment before deallocate_inode()
to explain this more clearly, since "This function deallocates an inode"
doesn't really give the reader much more insight than the function name
"deallocate_inode()". Maybe something more useful like:
/*
* This function reverts various counters and bitmaps incremented in
* pass1 for the inode, blocks, and quotas before it was decided the
* inode was corrupt and needed to be cleared. This avoids the need
* to run e2fsck a second time (or have it restart itself) to repair
* these counters.
*
* It does not modify any on-disk state, so even if the inode is bad
* it _should_ reset in-memory state to before the inode was first
* processed.
*/
... would be helpful to readers in the future. In either case, you
can add my:
Reviewed-by: Andreas Dilger <[email protected]>
Cheers, Andreas
>
> Signed-off-by: Luis Henriques (SUSE) <[email protected]>
> ---
> e2fsck/pass2.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index b91628567a7f..e16b488af643 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -1859,12 +1859,13 @@ static int deallocate_inode_block(ext2_filsys fs,
> static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
> {
> ext2_filsys fs = ctx->fs;
> - struct ext2_inode inode;
> + struct ext2_inode_large inode;
> struct problem_context pctx;
> __u32 count;
> struct del_block del_block;
>
> - e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
> + e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
> + sizeof(inode), "deallocate_inode");
> clear_problem_context(&pctx);
> pctx.ino = ino;
>
> @@ -1874,29 +1875,29 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
> e2fsck_read_bitmaps(ctx);
> ext2fs_inode_alloc_stats2(fs, ino, -1, LINUX_S_ISDIR(inode.i_mode));
>
> - if (ext2fs_file_acl_block(fs, &inode) &&
> + if (ext2fs_file_acl_block(fs, EXT2_INODE(&inode)) &&
> ext2fs_has_feature_xattr(fs->super)) {
> pctx.errcode = ext2fs_adjust_ea_refcount3(fs,
> - ext2fs_file_acl_block(fs, &inode),
> + ext2fs_file_acl_block(fs, EXT2_INODE(&inode)),
> block_buf, -1, &count, ino);
> if (pctx.errcode == EXT2_ET_BAD_EA_BLOCK_NUM) {
> pctx.errcode = 0;
> count = 1;
> }
> if (pctx.errcode) {
> - pctx.blk = ext2fs_file_acl_block(fs, &inode);
> + pctx.blk = ext2fs_file_acl_block(fs, EXT2_INODE(&inode));
> fix_problem(ctx, PR_2_ADJ_EA_REFCOUNT, &pctx);
> ctx->flags |= E2F_FLAG_ABORT;
> return;
> }
> if (count == 0) {
> ext2fs_block_alloc_stats2(fs,
> - ext2fs_file_acl_block(fs, &inode), -1);
> + ext2fs_file_acl_block(fs, EXT2_INODE(&inode)), -1);
> }
> - ext2fs_file_acl_block_set(fs, &inode, 0);
> + ext2fs_file_acl_block_set(fs, EXT2_INODE(&inode), 0);
> }
>
> - if (!ext2fs_inode_has_valid_blocks2(fs, &inode))
> + if (!ext2fs_inode_has_valid_blocks2(fs, EXT2_INODE(&inode)))
> goto clear_inode;
>
> /* Inline data inodes don't have blocks to iterate */
> @@ -1921,10 +1922,22 @@ static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* block_buf)
> ctx->flags |= E2F_FLAG_ABORT;
> return;
> }
> +
> + if ((ino != quota_type2inum(PRJQUOTA, fs->super)) &&
> + (ino != fs->super->s_orphan_file_inum) &&
> + (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
> + !(inode.i_flags & EXT4_EA_INODE_FL)) {
> + if (del_block.num > 0)
> + quota_data_sub(ctx->qctx, &inode, ino,
> + del_block.num * EXT2_CLUSTER_SIZE(fs->super));
> + quota_data_inodes(ctx->qctx, (struct ext2_inode_large *)&inode,
> + ino, -1);
> + }
> +
> clear_inode:
> /* Inode may have changed by block_iterate, so reread it */
> - e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
> - e2fsck_clear_inode(ctx, ino, &inode, 0, "deallocate_inode");
> + e2fsck_read_inode(ctx, ino, EXT2_INODE(&inode), "deallocate_inode");
> + e2fsck_clear_inode(ctx, ino, EXT2_INODE(&inode), 0, "deallocate_inode");
> }
>
> /*
Cheers, Andreas
On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <[email protected]> wrote:
>
> This new test validates e2fsck by verifying that quota data is updated
> after a directory optimization is performed. It mimics fstest ext4/014
> by including a filesystem image where a file is created inside a new
> directory on the filesystem root and then root block 0 is wiped:
>
> # debugfs -w -R 'zap -f / 0' f_testnew/image
I appreciate the test case, and I hate to be difficult, but IMHO this
test case is not ideal. It is *still* reporting quota inconsistency
at the end, so it is difficult to see whether the patch is actually
improving anything or not?
This is because the image is testing a number of different things at
once (repairing the root inode, superblock, etc). IMHO, it would be
better to have this test be specific to the directory shrink issue
(e.g. a large directory is created, many files are deleted from it,
then optimized), and ideally have a non-root user, group, and project
involved so that it is verifying that all of the quotas are updated.
Cheers, Andreas
>
> Signed-off-by: Luis Henriques (SUSE) <[email protected]>
> ---
> tests/f_quota_shrinkdir/expect.1 | 40 +++++++++++++++++++++++++++++++
> tests/f_quota_shrinkdir/expect.2 | 7 ++++++
> tests/f_quota_shrinkdir/image.gz | Bin 0 -> 11453 bytes
> tests/f_quota_shrinkdir/name | 1 +
> 4 files changed, 48 insertions(+)
> create mode 100644 tests/f_quota_shrinkdir/expect.1
> create mode 100644 tests/f_quota_shrinkdir/expect.2
> create mode 100644 tests/f_quota_shrinkdir/image.gz
> create mode 100644 tests/f_quota_shrinkdir/name
>
> diff --git a/tests/f_quota_shrinkdir/expect.1 b/tests/f_quota_shrinkdir/expect.1
> new file mode 100644
> index 000000000000..812fe44b887d
> --- /dev/null
> +++ b/tests/f_quota_shrinkdir/expect.1
> @@ -0,0 +1,40 @@
> +Pass 1: Checking inodes, blocks, and sizes
> +Pass 2: Checking directory structure
> +Directory inode 2, block #0, offset 0: directory corrupted
> +Salvage? yes
> +
> +Missing '.' in directory inode 2.
> +Fix? yes
> +
> +Missing '..' in directory inode 2.
> +Fix? yes
> +
> +Pass 3: Checking directory connectivity
> +'..' in / (2) is <The NULL inode> (0), should be / (2).
> +Fix? yes
> +
> +Unconnected directory inode 11 (was in /)
> +Connect to /lost+found? yes
> +
> +/lost+found not found. Create? yes
> +
> +Unconnected directory inode 12 (was in /)
> +Connect to /lost+found? yes
> +
> +Pass 3A: Optimizing directories
> +Pass 4: Checking reference counts
> +Inode 11 ref count is 3, should be 2. Fix? yes
> +
> +Inode 12 ref count is 3, should be 2. Fix? yes
> +
> +Pass 5: Checking group summary information
> +[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
> +Update quota info for quota type 0? yes
> +
> +[QUOTA WARNING] Usage inconsistent for ID 0:actual (4096, 5) != expected (14336, 4)
> +Update quota info for quota type 1? yes
> +
> +
> +test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
> +test_filesys: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
> +Exit status is 1
> diff --git a/tests/f_quota_shrinkdir/expect.2 b/tests/f_quota_shrinkdir/expect.2
> new file mode 100644
> index 000000000000..814f84a54fd6
> --- /dev/null
> +++ b/tests/f_quota_shrinkdir/expect.2
> @@ -0,0 +1,7 @@
> +Pass 1: Checking inodes, blocks, and sizes
> +Pass 2: Checking directory structure
> +Pass 3: Checking directory connectivity
> +Pass 4: Checking reference counts
> +Pass 5: Checking group summary information
> +test_filesys: 14/256 files (14.3% non-contiguous), 1146/8192 blocks
> +Exit status is 0
> diff --git a/tests/f_quota_shrinkdir/image.gz b/tests/f_quota_shrinkdir/image.gz
> new file mode 100644
> index 0000000000000000000000000000000000000000..753774f6a11f8a60fdbdf2ce205e64a36d753893
> GIT binary patch
> literal 11453
> zcmeI#YgAL|xd335ai~RPTIz*^A*;1VkEMb$7%yPVayrU5$Q=SCgrEpWB1A45hF}tu
> z3Y~H*WdsERqH+nj!C*iFAtIxUM=oR8*&De9N+r(Tgb2(g5J+~%uCD$#tH+*yUCWdG
> z>s#OZ=l!1VUGMj9V*ZbQH1xa8ahq%s63@lQZkQXly%`x>n?3zvVAIw2zEfTQMdN?n
> z4!`*0mQQ{b@-w?%9o~BN+n?8F1biL!!S+)DJwM(tw|V<X$p=3<^*Go5@wLWi_YUoE
> z3xE4(j2rmgV)xLJ{#IFkb!)0+$?){-`!><Fq@dvUbAo9uEu+zw^((%DBe7jcRjGge
> zg-34T*4e>{g^H5Q;?4)Coh9@)rBCFSH0ST{P0Jki8N799=~_bYkWjf65kHqQeZP|L
> z>^wA?r7kp1#xG0hw-ia~&%WO5Mr>PpQ7+_L(#6$jk3GE#&r%Z5l5Y9a8KXTslU>%z
> zTJhZ-Zma15J|%drF9Dwu0==O?;QPYE(4TUgIm-P<Cd=%?&3=oEw*q2Fynb1l_kBMp
> z9kpk|VuqJNw`IfIKfB6~c8~nkl81>eZ+5UJTAtWWY2z(?{=yge{WrCb4sD1cE8JsX
> zIbO9OQ^#LA=xMoGQeh!-E&CO{BMHN~1NgD=dfV)nwXYVV59;Dj%11ZH;krIz#^teB
> zzw^AeP1=jRZV?Qb+%FF5)6B(}7{%hd$ScqKB9)_rVz>I<<rS0m!lk*{8T^~*O8RPs
> z0mnZ#<6%$K%Zta_n=L+PyQY(EK}iL}7CPDcVaqnrZ_sAXuA*H<+kp@Q`8=*$)^|;>
> z$CuJ8@WO2l`GR(~y<tR}F2AMl)<<M%CKdjA`r5?rXS9hbe5eMWSYM%-jH}wqnd{P{
> z(9pQ~nkx>kYM1w2@{-SH_v&gWJG%~4WzN~p4EpLQIh!FII*R?}yHblk%3Olfb!5l4
> zbG|(~rDg~Dx)c;7fhqT8l?OeQ&ri46{rb?8OPiW;n`Oa6i^B-Mag7p<+s@(HX5!yA
> zr;5%$TKS;)(*v}8&DXT9vR(an!Nch03lnu^O?B*Bw6-$wOZIhI3$^J6`vy%x6>Hdq
> zv{tBTzoO080T0zBcC50KZf-0XP#^v1#P0F_`tmA|gxSr*_0?X0v2}3d<)eg|V%2~8
> zyii`w8^QC|Uca6Zpq8Ax!YJ*?^vtuEJ`P=QrDJeK-&f3Ja;_ZXUFqmqNx5+4vyJ21
> z`Wa}=xWjmY)Pw&m(WSXKTtAwr+mwC%**BC9oM>_EiEAVLaH0o$j&WGb!%kb>P9&E&
> zEp=L|pJ(E^URoKcE2e!^6$iN};`O0f@CgM+Z*)J7e_0u+o?hiRhxw~4Q->RLLhU+6
> zB=oBf?b<DHo!sM*YP6HBYav0N-hU)LDyG5L`2>DBvr>q63M2a6wzS`f$u*!6uW`KU
> z_yY@$@9u_mO<Nqj;|!kc<$_yTdRB0c5FXG8?DAHZ)cKslxbxcDI_LAG3nsy<Rowty
> zVHl`cnt;aeo6}y$sNHp|hHtuzSKP>!2RUP>y}_RY7YWq7VsjQGIGLK%fC?_4+(<&G
> zH~b+mCyX|G<2?(4m1^xvTc3h?g+cPdd2qz|kXO&dU8dxI;c02m)y}gunb{vY&c7Nw
> zzMPw^tDn(ci}_lbj65CQXRa9(fL%+(1Lg};6vM0O-Ho}u50gn(Yz(PL2k<dj?kCZ~
> zcF1+?4mVkKFN)?aE{9!_d)!!Iq`WB%er^Ax0c4{<@51dUW`pKCESDRj5)JMuI)lw}
> zeN>7`Do36HMgZxm*2z$V%m^OCCb>+N`a}Rx;-JYx_h8RW;X*o5?ho!myG$WM=1#By
> zePlYzYdaBEBX<XPqIaf`^Nz+r<;WjRDGVCZX|M$O%0%O-x&zDMBPboyoBVj~vPOf(
> zh}vSkra*iDUHAk3N5>t4DbgCv5W2HhOGnB9Uvm?mp%$@Gr76)Y<1+<~6sQIn!pco-
> zvvjt|0oy~OIkF@>M5ov-72)~tPf4P^@E?J4T2q-Umi2-rE0b?$J*J7MvQU<WCZ)<r
> ztO1(19voG?X@439+dO)Eo|+-g0&{^OWmsZ}GrSitDE$*@M(LWyjBZp!Bu3G}GDRr4
> z*(HPyA0{bWA{|6|P%n!{?rM{IgL~MWqunl{#j-wd8#cm?)Q1<#_koV+8AUL;qdLf6
> zDnt{o@42VRt<|BOU@M!er+KjcXFZ3l!5c~9IQS|MK#0r(Yk_b=ls8xcfZwc~>&|(-
> zzjut<X#qS^hHbw866I7;D7&2cg`G2`Jxea_QwH%8K7Mb(m2NM?k59iQ0d_Uc%96r(
> zgG&1(%iysF6~-MAcr5)dugEl;zqm<9$yNWUziIkNax~%4x~lz$2AU`6&Lw6aJ-@OR
> zO(5;2aPGI1bw3@C`)>-5`Is=VK2+pqbXxSfw=jekSX_tKo${-W#&_hqF$SR=uK^2X
> zn)w1Rr&w5Pl(#p#cRKp(UlutzZ7m`YwiZ=J(;q&X9a+X_kQJ^Ux!tc(r}+X~=bltG
> zM}@e{%0VAA7F*<=5{AenrD!OIaD9bAaw)74VJ1(hpZo~ujQRqng%KQCJ4i<pv1P8e
> zFf<<aLK)aR*IyWHkQy~>s14R*3gA6B5k!<0YdWw6(<i*I2F3oGRp2B~=_VNh<Iz-X
> z)$}p1#jPoy?Z*?jiQM5j?1qWOpfbgB_z1FSisOm98CsD6+K#DAeDiI7IHk!I?ZZly
> zO?(<fTm!o!h$)s4tZrh$9As1}<I~k*Elfm;u?ADBxz0iph&xCZ$BCln6#+4df4oVJ
> zXGk4ol1?y4k#->X8I+>9A`NhbK2xMfgTxR^!IcKGAsVR;k^!s<ntYyy!tVp^%B+dI
> zhb38%igkDN!C_Gr%wr8JITP}ErbFWZYiaaOn?#CS#cq!(+e9L4iu)<~z6*62nN}o{
> z@3=&aX_m1@ZW_5hy|E9PVfAsZkZY^M{hNfyQ|wDFOCRYkZh$?ITbN|Du9`8-hARR)
> zHS?s_<wi1FKH9k~;<44E>TQh?q|R-U9AxV6ql)L9BFUTiBo17$eWwffUmb3IJo#=;
> z&jMD(O%etSi5<~Z>>Br+sxd0kT~rP|LtbLP_Y_4%?1ROK2CML_i=xZL!6*jMgi)Nv
> zBTyw$;MqDE$q}_fV~8Fr@)QrsGN4Xm0;~3Hn4}xTM%Wg~!D>yHdG}7xh~iXq2K&Nv
> zK^Rukw1i$Wr7^;5L_?4cS;DTH&ND)G!d0dqMi5ixgSuiGrKFq6lyktB*no-3>+BBJ
> zN?nmG%xpSoZsv!CLB;4kAk=)HPo>D2pay%UtmlWSWm-@HFwAZIFtwZl5>bClXYw&C
> zEYwu?!E*<9z4H}aUnGoPxA|As{!Yh}=2y1l^!%UZC^qsXH&G??lfrNUz*W^whI5)c
> zkbZ8es&10T5x2u$h{UsVGQ!Z50cRmqT#iaU$s{&9KzT?nR${uylMG6W;eALc5XEa6
> zl-nW0z&T$133`oq2;Pb01A&alRKy23!|QO9%E2)7trHX@f+DK{L$P5~gt?0!7zVqe
> z6{b+Lk{=9105)b~n7b_j0trP^gSum|i9R6h8Hqj^beB~7gjGVjzeD_m;`0N6rL2?#
> z5gDR7)_1h}GU-d!Dy^0(xxso#tD{OaER1#+k|<c8Nf{`pR76cEAh|t~1Q?axiQz^`
> zmgck~IWfp6TLbF>t&%yRb_uA|%qmVLM$k1j=t%{T7#xiDV1M9VBunZwz3egWC*-f%
> z#CkLkd&NDg4=9%QX^OBF?#JYo>JTqYDY_p6M?0#i{_=Rx1C3DlkUQ&P8R>4ic!C|L
> z56pt16dC&9HHfbe5CU3R>4b3GLZ_j3x;p>a;j+inv5==>v81f;Qp-wL?hb;Q^OH*3
> zd2sK@d(GeeN9&g8+Snq8f^~szOXZ97%ibiG<HL{HIiXc}-t`+xc=PKN3vox(>`L~V
> zb@H2asgCrK^7%^m!OS0iKLf+c<`zR8_ZIa}0??e3hjCO>w&|bcVcoL|SOu&CRspMk
> zRlq7>6|f3e1*`&A0jq#jz$#!BunJfOtO8a6tAJI&D)4U?m=cWU*!=WO8YgjP`<{S}
> Z|MrZm3H(16$j;kv=2v{eh7H*p{tBVyta$(c
>
> literal 0
> HcmV?d00001
>
> diff --git a/tests/f_quota_shrinkdir/name b/tests/f_quota_shrinkdir/name
> new file mode 100644
> index 000000000000..8772ae5c814b
> --- /dev/null
> +++ b/tests/f_quota_shrinkdir/name
> @@ -0,0 +1 @@
> +update quota on directory optimization
Cheers, Andreas
Andreas Dilger <[email protected]> writes:
> On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <[email protected]> wrote:
>>
>> This new test validates e2fsck by verifying that quota data is updated
>> after a directory optimization is performed. It mimics fstest ext4/014
>> by including a filesystem image where a file is created inside a new
>> directory on the filesystem root and then root block 0 is wiped:
>>
>> # debugfs -w -R 'zap -f / 0' f_testnew/image
>
> I appreciate the test case, and I hate to be difficult, but IMHO this
> test case is not ideal. It is *still* reporting quota inconsistency
> at the end, so it is difficult to see whether the patch is actually
> improving anything or not?
Maybe I misunderstood how the tests really work. Here's what I
understood:
e2fsck is run twice. During the first run the filesystem is recovered.
And that's the output of expect.1 -- it reports the quota inconsistency
because quota data needs to be fixed. And it is fixed in that first run,
where e2fsck returns '1' ("File system errors corrected"). The second
time e2fsck is run (expect.2) it will do nothing, and '0' is returned
because the filesystem hasn't been modified.
Without the first patch in this series the second time e2fsck is executed
it will still fail and report inconsistencies because the first time the
fix wasn't correct. (And after this second time the filesystem should
actually be corrected, a third run of e2fsck should return '0'.)
> This is because the image is testing a number of different things at
> once (repairing the root inode, superblock, etc). IMHO, it would be
> better to have this test be specific to the directory shrink issue
> (e.g. a large directory is created, many files are deleted from it,
> then optimized), and ideally have a non-root user, group, and project
> involved so that it is verifying that all of the quotas are updated.
Right, that makes sense. However, I'm failing to narrow the test to that
specific case. I've tried to create a bunch of files in a directory and
used the debugfs 'kill_file' to remove files from that directory.
However, in that case e2fsck isn't reporting quota inconsistencies as I
would expect. Which may hint at yet more quota-related bugs. But I'm
still looking.
Cheers,
--
Luis
Luis Henriques <[email protected]> writes:
> Andreas Dilger <[email protected]> writes:
>
>> On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) <[email protected]> wrote:
>>>
>>> This new test validates e2fsck by verifying that quota data is updated
>>> after a directory optimization is performed. It mimics fstest ext4/014
>>> by including a filesystem image where a file is created inside a new
>>> directory on the filesystem root and then root block 0 is wiped:
>>>
>>> # debugfs -w -R 'zap -f / 0' f_testnew/image
>>
>> I appreciate the test case, and I hate to be difficult, but IMHO this
>> test case is not ideal. It is *still* reporting quota inconsistency
>> at the end, so it is difficult to see whether the patch is actually
>> improving anything or not?
>
> Maybe I misunderstood how the tests really work. Here's what I
> understood:
>
> e2fsck is run twice. During the first run the filesystem is recovered.
> And that's the output of expect.1 -- it reports the quota inconsistency
> because quota data needs to be fixed. And it is fixed in that first run,
> where e2fsck returns '1' ("File system errors corrected"). The second
> time e2fsck is run (expect.2) it will do nothing, and '0' is returned
> because the filesystem hasn't been modified.
>
> Without the first patch in this series the second time e2fsck is executed
> it will still fail and report inconsistencies because the first time the
> fix wasn't correct. (And after this second time the filesystem should
> actually be corrected, a third run of e2fsck should return '0'.)
>
>> This is because the image is testing a number of different things at
>> once (repairing the root inode, superblock, etc). IMHO, it would be
>> better to have this test be specific to the directory shrink issue
>> (e.g. a large directory is created, many files are deleted from it,
>> then optimized), and ideally have a non-root user, group, and project
>> involved so that it is verifying that all of the quotas are updated.
>
> Right, that makes sense. However, I'm failing to narrow the test to that
> specific case. I've tried to create a bunch of files in a directory and
> used the debugfs 'kill_file' to remove files from that directory.
> However, in that case e2fsck isn't reporting quota inconsistencies as I
> would expect. Which may hint at yet more quota-related bugs. But I'm
> still looking.
OK, I _may_ have found a simple way to generate an image to test my patch.
Here's what I came up with:
make testnew
tune2fs -O quota f_testnew/image
debugfs -w -R "ln lost+found foo" f_testnew/image
debugfs -w -R "unlink lost+found" f_testnew/image
echo "update quota on directory optimization" > f_testnew/name
make testend
mv f_testnew f_quota_shrinkdir
This will trigger a directory optimization after the recreation of the
lost+found directory. Do you think this would be good enough?
Cheers,
--
Luis