All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
@ 2017-04-15 13:41 Theodore Ts'o
  2017-04-15 13:41 ` [PATCH 2/3] tests: add new test f_quota_extent_opt Theodore Ts'o
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-15 13:41 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

If quota is enabled, optimizing the extent tree wouldn't update the
in-memory quota statistics, so that a subsequent e2fsck run would show
that the quota usage statistics on disk were incorrect.

Google-Bug-Id: 36391645

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/e2fsck.c  |  1 +
 e2fsck/e2fsck.h  |  1 +
 e2fsck/extents.c | 23 ++++++++++++++++++-----
 e2fsck/pass1.c   |  1 +
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
index 0f9da46a..4baedbac 100644
--- a/e2fsck/e2fsck.c
+++ b/e2fsck/e2fsck.c
@@ -169,6 +169,7 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
 	ctx->fs_fragmented = 0;
 	ctx->fs_fragmented_dir = 0;
 	ctx->large_files = 0;
+	ctx->clusters_allocated = 0;
 
 	for (i=0; i < MAX_EXTENT_DEPTH_COUNT; i++)
 		ctx->extent_depth_count[i] = 0;
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index f3568106..ecd3b1e1 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -372,6 +372,7 @@ struct e2fsck_struct {
 	profile_t	profile;
 	int blocks_per_page;
 	ext2_u32_list encrypted_dirs;
+	__u32 clusters_allocated;
 
 	/* Reserve blocks for root and l+f re-creation */
 	blk64_t root_repair_block, lnf_repair_block;
diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index c4167e16..1df31d71 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -208,17 +208,21 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
 static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
 				     ext2_ino_t ino)
 {
-	struct ext2_inode	inode;
+	struct ext2_inode_large	inode;
+	struct ext2_inode	*inode_ptr;
 	errcode_t		retval;
 	ext2_extent_handle_t	handle;
 	unsigned int		i, ext_written;
 	struct ext2fs_extent	*ex, extent;
+	__u32			start_val;
 
 	list->count = 0;
 	list->blocks_freed = 0;
 	list->ino = ino;
 	list->ext_read = 0;
-	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
+	inode_ptr = (struct ext2_inode *) &inode;
+	e2fsck_read_inode_full(ctx, ino, inode_ptr, sizeof(inode),
+			       "rebuild_extents");
 
 	/* Skip deleted inodes and inline data files */
 	if (inode.i_links_count == 0 ||
@@ -248,16 +252,19 @@ extents_loaded:
 	memset(inode.i_block, 0, sizeof(inode.i_block));
 
 	/* Make a note of freed blocks */
-	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
+	quota_data_sub(ctx->qctx, &inode, ino,
+		       list->blocks_freed * ctx->fs->blocksize);
+	retval = ext2fs_iblk_sub_blocks(ctx->fs, inode_ptr, list->blocks_freed);
 	if (retval)
 		goto err;
 
 	/* Now stuff extents into the file */
-	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
+	retval = ext2fs_extent_open2(ctx->fs, ino, inode_ptr, &handle);
 	if (retval)
 		goto err;
 
 	ext_written = 0;
+	start_val = ctx->clusters_allocated;
 	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
 		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
 		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
@@ -295,11 +302,17 @@ extents_loaded:
 		ext_written++;
 	}
 
+	if (start_val != ctx->clusters_allocated) {
+		__u32 num = ctx->clusters_allocated - start_val;
+		quota_data_add(ctx->qctx, &inode, ino,
+			       num * ctx->fs->blocksize);
+	}
+
 #if defined(DEBUG) || defined(DEBUG_SUMMARY)
 	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
 	       ext_written);
 #endif
-	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
+	e2fsck_write_inode(ctx, ino, inode_ptr, "rebuild_extents");
 
 err2:
 	ext2fs_extent_free(handle);
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 188cc562..99e8f66a 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -4017,6 +4017,7 @@ static errcode_t e2fsck_get_alloc_block(ext2_filsys fs, blk64_t goal,
 	}
 
 	*ret = new_block;
+	ctx->clusters_allocated++;
 	return (0);
 }
 
-- 
2.11.0.rc0.7.gbe5a750

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] tests: add new test f_quota_extent_opt
  2017-04-15 13:41 [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Theodore Ts'o
@ 2017-04-15 13:41 ` Theodore Ts'o
  2017-04-15 13:41 ` [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled Theodore Ts'o
  2017-04-16 19:17 ` [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Darrick J. Wong
  2 siblings, 0 replies; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-15 13:41 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

Add a test to validate the changes in commit 403bcb668e4f: "e2fsck:
update quota when optimizing the extent tree".

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 tests/f_quota_extent_opt/expect.1 |  15 +++++++++++++++
 tests/f_quota_extent_opt/expect.2 |   7 +++++++
 tests/f_quota_extent_opt/image.gz | Bin 0 -> 9018 bytes
 tests/f_quota_extent_opt/name     |   1 +
 4 files changed, 23 insertions(+)
 create mode 100644 tests/f_quota_extent_opt/expect.1
 create mode 100644 tests/f_quota_extent_opt/expect.2
 create mode 100644 tests/f_quota_extent_opt/image.gz
 create mode 100644 tests/f_quota_extent_opt/name

diff --git a/tests/f_quota_extent_opt/expect.1 b/tests/f_quota_extent_opt/expect.1
new file mode 100644
index 00000000..1b966213
--- /dev/null
+++ b/tests/f_quota_extent_opt/expect.1
@@ -0,0 +1,15 @@
+Pass 1: Checking inodes, blocks, and sizes
+Inode 12 extent tree (at level 1) could be narrower.  Fix? yes
+
+Pass 1E: Optimizing extent trees
+Pass 2: Checking directory structure
+Pass 3: Checking directory connectivity
+Pass 4: Checking reference counts
+Pass 5: Checking group summary information
+[QUOTA WARNING] Usage inconsistent for ID 0:actual (147456, 3) != expected (148480, 3)
+Update quota info for quota type 0? yes
+
+
+test_filesys: ***** FILE SYSTEM WAS MODIFIED *****
+test_filesys: 12/1024 files (25.0% non-contiguous), 1339/4096 blocks
+Exit status is 1
diff --git a/tests/f_quota_extent_opt/expect.2 b/tests/f_quota_extent_opt/expect.2
new file mode 100644
index 00000000..0bd4632c
--- /dev/null
+++ b/tests/f_quota_extent_opt/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: 12/1024 files (25.0% non-contiguous), 1339/4096 blocks
+Exit status is 0
diff --git a/tests/f_quota_extent_opt/image.gz b/tests/f_quota_extent_opt/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..21dcfbdfeb150a46a6746de21ad874a9eff12185
GIT binary patch
literal 9018
zc-rlleN>ZIn!s&4TV~g7(Ncx_K}K!Wf<P%E0!`vnAXBU&3a0XrOywg|3@Ic6CWdtC
zFeqQqDnvd^YEnc*NJ)eMA;ebs3X=2{F}^^8NHGCkNFgt8BrnN3&YnGc=A1pdXXng0
zyL)!|opbMB_x|(T=icW&KU~qTSC#&L{<BwKn>D=heW~r5dEQoSJrWGW#Ds;vl{s`X
z_4ciqTkTo5wo8tu%j!}MxBFs~8qynX#U*`sy!ml5{c+sowf=R5nXx5-$LQj%5e>0P
zxBVZV&wRNe{r2hbHRPcXn@wo474|lLBAtqzUwW`?Ju9*Wn?ACf(V`s|E5U+B*8OU<
zMdLqwB{DQg*4!BrLMD3+C8#=>r7zmsBu3BZcLHm=yQQ76gYfCHrD?%2wMFX@aN?iO
zH`TPn*w3x6dDt3c!N&O^d6Ug&k~+tJq4}wdU(OD_C?Qtwo$TgSxQyj|QeSWHw49yS
z9Z-8D2p8>r@}!(Af=7>bUx_oxG?r*yOJf={rYqKOQ8TsU#92bO!QWU&hiwCm>H*ZZ
zE-mA7PrjOJ1zMLLf#))gZ&MFXMJey!$tnKrK@@9iA5K69$(@R)2?iRj&1maHG*@*`
zzFzg8b2etTlO4B2!4sY;u4WGCr5!&sd?=ZuFPKC?FTG(;#;+O<1UBQ7;#hRL?rK{=
zdtzN@_K!iGfs^ti^*qU~C#JkCa%5&Yy)$rX=CXQ`lZ5sBag2%mqeA>d;JSHf3U9S^
zq+&!08N%+8I2f;K+yzrTG)<p!Z8abm4}E@B-GZHS;J(LRLwI+cu1~O6P~z_~7YO^x
zfookvumS4wk3U}ZaTk==A9tG=*4^dG7DSm0BX$pWdRdch?LMm>*HPH2Eo!82!RO8!
z)1AsM%Hm&D$VV}6^vlT!qN-ki)iP$Q9zPK1pYJpTG>^R2q>$EY`Pc&4$^5i4;To=4
zGXCYKug98D9{uM94ZlIl6OU9bk{c9vPER$B6@Ma_UOvFHYkodbH)Nk)R&P4#vBoN;
z2B^m?mx8v=j9qhSG?}Qw&bIET(b1WwZcn~+1F`Xzq4!+v-Y(PBMk1M9g-+_`oW`%2
zr`6WJnOViT&d}!prQbf4j89+Am8zdSdC|#C(WBj4QZ~B2sB!$fU39(bgF7+9%QfdI
zH4jIl8T^~2({<W64-OtXh&2%7twQ}CFKm6Beq#d0N6RAno$rcAZ&fVnuS};GE$VkS
zs5i~<Y!o&(vpLe+%cckUJ<r|%-Nj?Q&Shc)LqBR4IPyn+lW!Yy<}KRKs$%S8We(}+
zsC}l;4JBt_76bOhLf0QR&e!S6A_KfoYQYgXN=?Brm?@Z)GiKM97p0{vf1l8?CVHt-
z?YLMF=IYc{p+OcWDJaL$LmL}i#1yNSY?fHxAe)WR`h!S4`C2(Pt2Wsujy)c=7gbTL
z3ia@_&+F6Tg7a-l9yvX^-fup_ock$ii}O1>BhPLlj%Ocn*oakgF@L$1-cgfDwY=K7
z${?7f%^XT%9jt4buV)PH<{E~2Ru5U~M{^#Wj}Gn_Tc6{_`$-Ji-ca>0mdEeEALNY3
zzvw&=mpEkOnu>T7d)L#2+xjTsW~HR#yBjyIPZ1i}%ghN|6Ww{w>J>ZL`patErIAaD
zQe%56HN!?$bgMD!fbs{QfQckU-@&_Tj8|1p4DHyxT-j5<xRlWHaHFP+IULE@<<9u0
zM)*e7{FAt;fyrkFbIOcIe1$t=*y(U0IHKv2B-JQLiUDJE<43xkY#IxN5+`reP99W+
zwkIJ%^C6HU!Sx!+zHc7&QmGR<E`9&dMbmYrJrY+uSI*tp&Oye_o*+q#(-Jlq0?^Oe
z_O8DP&u7h7rqx7++4>0Y7=q9kmKqua9CF>2q6u-NsL1mwm(aci{kSa`-}2C1;zFYq
z=Iuta;EU7q;hU)V^2Ck!-O5e)UCI=A0&EbAdK>wkLPLnX4^oOr$%+ekUP?OL3uOaQ
zGzH(Y%&>*c%I-1ygV)48sl>y<+aJtr!V{DS;6Fl*fSX(y9;KP{#0M)$@D->)Qk!Zk
z7M2^l?G=y_(52cg2!#ekc;<t+sXQP+j*@g5&ozz&<gOV6-vD$p*FxcC{9bkjA_1(j
zf-v<($7k>|bRAeND+ni#T}5-@elRM`PvhyS+Kcv?-vhUa1*yb!s$HC9cmb;Q*3XXg
zIA^CFyl~%*>-vwS@=GaY`0eb|$P;rQI3d=j^7(mzoR8sl=mIblw0x-0co84WPC~Aj
z4}wP}LA0p!=;{M?Z~zc57nK;x@pv`|DK>8g1LEJ-(Y^5AY%;<GlyVx$(w9eIGmvUC
z7i3CoG+hzlqT^%u2WSZ>khzE9v=k5SyXd4@0v3t+DSo8{q3T_G81m438SEEFrTFm)
z*Hl~WpCTGF6Ra2OXwuToi>mGR6N{Tt;Tni6@uNx8qr(O#e#@dGe_G#V72N1KXD$b;
z#PKPN(ng)}Ekg+BUouKOlFZ;|RH?SLCn8!i0xHF!$(GW&tEwICM-ijhMP>`n(He1v
z_t1STJ#-E*$Xvs7bc9WYZRqdddmu@|NyQb-m2p36&p|Gj3E&P1nkp)s6L3S?c}Ru%
z70@JhNfq%kdOO$PccLfYr_cd_pGqzD{3h|t#k^h0J?xX&!R*hn`(GApUx$4N#U|Qr
z$L`{6RT9{_*)!%Ha!aA-CBt@m9n=DBlaGWGuNesJnCt>G0B)A_B<B=*Ryp>;t&p~C
zrqb{p`{QiA`2uJV??}nv3#$#=*pbLR;HaDv5!FAp&hcrs#Y~f<VNtrdjreU!D*OO)
z0&<y47*T-#fSsMaAGU($#HM7|Qel<jB-{aQ0sNAQ>+wX+N%(8%b6~X`$IlB!FPOu@
zdlGjV@g3E>oG|zyR4W#w@QW!z2MaEQq5*F?i6qr{x~W1qr{GE`6G)f$kkpm<E$l?(
zzIhS6EDlPJDm7lkzsEj`JTeb}gJMN8(M=VIf@V56C{fVJPDVzkj22;eAY7;NM%&B=
zP%g106LG2_&LOxHY6DQYDIC|Ex5fT@WZAqQTrGB``4tnsab&>&6b|&r+(|ec<t^@3
zP8!??r2w@u0ZF8#tmSU!q`+-ZEMSt8Ntl*_J^$J+_i2PM_bjmTvuuA3{onHEc(q45
z`)un!I2mUoholvu_Vi5ZH-oqS;OF=@&8f&@H&p1`&L$jE9%SS#J#syx$}1;gRHpsd
zA-_quG{1{w{^kUmr`11)2Z+8mM3fiCnP}#~9k!5(aJ%4%V6_>y(EQVMde${hFKrox
z>AaJ4D9W^ONWY6`%9;rB8q7~GnjP8dN3}-yQ&kr>X(tzlX32TB>dd;__gnh#!SolS
z76J<%Az!17I`~}{sw4F0ipuGYn{MsMm}?Mr8slzD*=Oq2-p}jx%Kgr#p^e8ezYRrW
zwRUfcb}9c%p~Bx(KVoBPEok(-l_#{?mJ1pBs`M`^TXd&OZmZ@N)SDKxe5;j5Cg^Q!
zbb?7dd2O4~Gf-h^?4mMXUzb5|!xX)Otc5x06tm?n>4`ewY-U%i2$S~ftU(ofO=CNP
ztUhGInA$R+uB-NHUpV9V{HUNsW#4;3|A4$f>OAYb>!6WLrlr5s|BP~uWh8fU^HU=>
z<4|MjGni$7_j3GfC1Ud?UffUu7bEK-6E8Kv6hOp1w-Wnt^B%um^*_kn#i6d9Jtx;n
zQi}OqC)Z7sdMx_S6$2sNJxQbMCfxjo@3)3VjChEO0pD)C|8P%hP(*=;s1We)=K6<p
zwGz8u^Y<(Ug1h(c!7s<%Y`aC}G&MD~q32WIXl$PUiaH$i`snCllt-Kw-FYNTdL(V`
zrh*hhYP!=h<#A*0TZI$f$^#Gf{4Ta^;(B9Hs}pDQE-QC@GWoPVHeg2je)FyOA5#bK
zINLwNl(vU6MfI_jYu)KDA0kIYfMbalH`ds;=*PQH|0P@hVkT{pv-skxu@l{aRQ1B^
z3QcjjqEEz&*rsUq&D>_*oOHOjVbL^wi)1BoCpoLNLtLbtzecM1@Oi)-e*Ez<UiQAU
z+$lp+pJr{UIv1|0Fc8_tkgv=pnM;IDn{hsmz&Q+8Lq7maIj%$~z`twHhkC>&nrjK=
zB7PV9b7TVWqY>Atc5-szXV4CbAen!jQt60+WzZ#{UoI-8T*7;?QxLN`7EBV8Y5Wq(
zWxOw&j=V6F<vYTp{X!2FfpY}@6VwN6l-GtaG{Oz4U``xd4$;JlRN_XJ4~GpSkOt6{
zLKQ%$oEC1;<oU955Xg)NbtKDx&`ssX$$*{E8NfwGJx{obC)y7qUz_iNt0nwo^(O9a
zPBh#EbpbtcL71qQvVr>{=QvykO#-!Ya+s)(@($OR6ALdvLq4-3xbG%{X5t@-=&dr0
ztb47|k1TYoS6xr&*mHQQWb)bKA0ioE?z&=cRa-*Gp~F)?lh4{RYZT>0`qf^lhJ=pP
z!&9@9&rY9<wCr@3ZjNEo+HxJ=K2&PD(tllMO!TFYZo<N>`M8g3Y%jOCd8woc9XAe7
zIe<i&>y;wYR2jj;;Da7!*`Rv>Ud9h2R&#f^(~zdD!L+Y$5GcN-6<*w}?NLadxf84v
zqbZ`&IT3d|dmqwf{sU-|Y?S%E{P{FRQj0ySx$oNns2|vW%S4aA?>L!Fhkt^y;P0Vw
zfKO8t=Kbwe3Yd^9!=ifUHsJl0@$h%h9B^6A35)8>+spnUTW3xKRbo-!+-AqAY>{~i
z<i?NYp_tu5T+mgiU*zZ4oXBB>bYVQ_Oimtpu9I<pMDpAz7_(U2Y~OyFuKWUHgs2?n
z)FFe5_aMwc;;|bWTvn~H!V3RaIs5jg<LPrO)@|?|hvnpkbw14FtE*fH52sJP+Db2@
zznp@*UA}0X)>BTMTK-Ar_PTyJ_sb%g?bVggt+2xXKQQUEZqJoJQ3I)NN!+%WTLZ>5
ziCUuv@FC|Ys~#Ev7QuLNJdIJ(>CK5}RY9`=U7i!B=r_J&AaKa60>}VtmfON|G{%jN
zeMqNS3!)NbGVc6bCHI5&c;vcy4lIyxl5rRE_M#_QIw%QH$@nCjmax{a15IX)K}P|H
zEQ(~)5!~><K@;G+kQMlgRCG!C>;IPQfKOyik1SnMkSdTAq$rB>K2pZQZr~ztPaaPq
zR&dGid1wz1m7*x|tT6c6ryvB7$U?*T+BpyWcI8p{3FIQ~Ny#bo6#XqyVB{GxV$<`m
zOT0#6=(5JC-m2)j`DFdy-_PsL8!zDl*fiv}IT6%_TeQN>svYQ(Sp_o1wiI27QH1xi
z??YAtE^-qIr=hIj?&W+AUxEY@eyU#~p<K1so{M~Ab^{McqEh{e2xY2|>@1|v8~}Pt
zbSct8f=Cr&{}`z>7lG*#zZCTbZZIdYXltdXt2kXA6ef@dMJOH^-!^!0m@F<*Zhj2L
zODr1Wdc$@OhcyCi24-bB5el7gonZ?nht&h&fon2b1o5KEubqiF&Ay<ESeg1CI7F5d
zngCStfA0^&Tzd)b_z%#tto=y7`M2O-yF*BG4o(7_f&KD+;GQf#g0Ie*($DDLeAzJQ
zdQ%)H!2hjg=D*=2+iQ|g*N5{}i$2r3mX`sc_R<yGTMS)LGa!&rOFFAnjWTY!b$)#P
zlgIyRX6CP6{JYJ}=|i+J@~1?hwa9t>YMz&gpToYHR!MyfZ<0>6d^0%M)-f2^Kh`$-
zqC~?QA3lK|bFI~PL_Z)iwiYnYa8ILUMN`*;$F|hke!er#RHHuu3_;}SA?hg`alM+i
zX!)h-#IUc4*}`HR3T5~_CaXGz#@1PfW(-?+ziigu#@tJ~EJq#d{7lO~?&-=e38or4
zYngdt{6Z0CtyY`JXO+f;ql0(2jp@m=rv=#L#hg5dvS#HyUtxt6R#;(${~BLW^Rd-$
M7Ki9pt=hNh&y+Za^Z)<=

literal 0
Hc-jL100001

diff --git a/tests/f_quota_extent_opt/name b/tests/f_quota_extent_opt/name
new file mode 100644
index 00000000..8c5d7a90
--- /dev/null
+++ b/tests/f_quota_extent_opt/name
@@ -0,0 +1 @@
+extent optimization with quota
-- 
2.11.0.rc0.7.gbe5a750

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled
  2017-04-15 13:41 [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Theodore Ts'o
  2017-04-15 13:41 ` [PATCH 2/3] tests: add new test f_quota_extent_opt Theodore Ts'o
@ 2017-04-15 13:41 ` Theodore Ts'o
  2017-04-16 19:24   ` Darrick J. Wong
  2017-04-16 19:17 ` [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Darrick J. Wong
  2 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-15 13:41 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

Add an extended option, -E no_optimize_extents, as well as a
e2fsck.conf profile option, to disable extent tree optimization.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/e2fsck.8.in      | 4 ++++
 e2fsck/e2fsck.conf.5.in | 4 ++++
 e2fsck/e2fsck.h         | 1 +
 e2fsck/extents.c        | 3 +++
 e2fsck/unix.c           | 8 ++++++++
 5 files changed, 20 insertions(+)

diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
index 915273d8..4ad575f4 100644
--- a/e2fsck/e2fsck.8.in
+++ b/e2fsck/e2fsck.8.in
@@ -226,6 +226,10 @@ option may prevent you from further manual data recovery.
 Do not attempt to discard free blocks and unused inode blocks. This option is
 exactly the opposite of discard option. This is set as default.
 .TP
+.BI no_optimize_extents
+Do not offer to optimize the extent tree by eliminating unnecessary
+width or depth.
+.TP
 .BI readahead_kb
 Use this many KiB of memory to pre-fetch metadata in the hopes of reducing
 e2fsck runtime.  By default, this is set to the size of two block groups' inode
diff --git a/e2fsck/e2fsck.conf.5.in b/e2fsck/e2fsck.conf.5.in
index 0bfc76ab..94525baf 100644
--- a/e2fsck/e2fsck.conf.5.in
+++ b/e2fsck/e2fsck.conf.5.in
@@ -205,6 +205,10 @@ of that type are squelched.  This can be useful if the console is slow
 (i.e., connected to a serial port) and so a large amount of output could
 end up delaying the boot process for a long time (potentially hours).
 .TP
+.I no_optimize_extents
+Do not offer to optimize the extent tree by eliminating unnecessary
+width or depth.
+.TP
 .I readahead_mem_pct
 Use this percentage of memory to try to read in metadata blocks ahead of the
 main e2fsck thread.  This should reduce run times, depending on the speed of
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index ecd3b1e1..a4b5fb24 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -169,6 +169,7 @@ struct resource_track {
 #define E2F_OPT_DISCARD		0x2000
 #define E2F_OPT_CONVERT_BMAP	0x4000 /* convert blockmap to extent */
 #define E2F_OPT_FIXES_ONLY	0x8000 /* skip all optimizations */
+#define E2F_OPT_NOOPT_EXTENTS	0x10000 /* don't optimize extents */
 
 /*
  * E2fsck flags
diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index 1df31d71..d9410028 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -518,6 +518,9 @@ errcode_t e2fsck_should_rebuild_extents(e2fsck_t ctx,
 	if (eti->force_rebuild)
 		goto rebuild;
 
+	if (ctx->options & E2F_OPT_NOOPT_EXTENTS)
+		return 0;
+
 	extents_per_block = (ctx->fs->blocksize -
 			     sizeof(struct ext3_extent_header)) /
 			    sizeof(struct ext3_extent);
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index b7322bc6..b6025535 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -709,6 +709,9 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		} else if (strcmp(token, "nodiscard") == 0) {
 			ctx->options &= ~E2F_OPT_DISCARD;
 			continue;
+		} else if (strcmp(token, "no_optimize_extents") == 0) {
+			ctx->options |= E2F_OPT_NOOPT_EXTENTS;
+			continue;
 		} else if (strcmp(token, "log_filename") == 0) {
 			if (!arg)
 				extended_usage++;
@@ -1007,6 +1010,11 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
 	if (c)
 		verbose = 1;
 
+	profile_get_boolean(ctx->profile, "options", "no_optimize_extents",
+			    0, 0, &c);
+	if (c)
+		ctx->options |= E2F_OPT_NOOPT_EXTENTS;
+
 	if (ctx->readahead_kb == ~0ULL) {
 		profile_get_integer(ctx->profile, "options",
 				    "readahead_mem_pct", 0, -1, &c);
-- 
2.11.0.rc0.7.gbe5a750

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-15 13:41 [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Theodore Ts'o
  2017-04-15 13:41 ` [PATCH 2/3] tests: add new test f_quota_extent_opt Theodore Ts'o
  2017-04-15 13:41 ` [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled Theodore Ts'o
@ 2017-04-16 19:17 ` Darrick J. Wong
  2017-04-21  7:37   ` Theodore Ts'o
  2 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2017-04-16 19:17 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

On Sat, Apr 15, 2017 at 09:41:16AM -0400, Theodore Ts'o wrote:
> If quota is enabled, optimizing the extent tree wouldn't update the
> in-memory quota statistics, so that a subsequent e2fsck run would show
> that the quota usage statistics on disk were incorrect.
> 
> Google-Bug-Id: 36391645
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/e2fsck.c  |  1 +
>  e2fsck/e2fsck.h  |  1 +
>  e2fsck/extents.c | 23 ++++++++++++++++++-----
>  e2fsck/pass1.c   |  1 +
>  4 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
> index 0f9da46a..4baedbac 100644
> --- a/e2fsck/e2fsck.c
> +++ b/e2fsck/e2fsck.c
> @@ -169,6 +169,7 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
>  	ctx->fs_fragmented = 0;
>  	ctx->fs_fragmented_dir = 0;
>  	ctx->large_files = 0;
> +	ctx->clusters_allocated = 0;
>  
>  	for (i=0; i < MAX_EXTENT_DEPTH_COUNT; i++)
>  		ctx->extent_depth_count[i] = 0;
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index f3568106..ecd3b1e1 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -372,6 +372,7 @@ struct e2fsck_struct {
>  	profile_t	profile;
>  	int blocks_per_page;
>  	ext2_u32_list encrypted_dirs;
> +	__u32 clusters_allocated;
>  
>  	/* Reserve blocks for root and l+f re-creation */
>  	blk64_t root_repair_block, lnf_repair_block;
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index c4167e16..1df31d71 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -208,17 +208,21 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
>  static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
>  				     ext2_ino_t ino)
>  {
> -	struct ext2_inode	inode;
> +	struct ext2_inode_large	inode;
> +	struct ext2_inode	*inode_ptr;
>  	errcode_t		retval;
>  	ext2_extent_handle_t	handle;
>  	unsigned int		i, ext_written;
>  	struct ext2fs_extent	*ex, extent;
> +	__u32			start_val;
>  
>  	list->count = 0;
>  	list->blocks_freed = 0;
>  	list->ino = ino;
>  	list->ext_read = 0;
> -	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
> +	inode_ptr = (struct ext2_inode *) &inode;
> +	e2fsck_read_inode_full(ctx, ino, inode_ptr, sizeof(inode),
> +			       "rebuild_extents");
>  
>  	/* Skip deleted inodes and inline data files */
>  	if (inode.i_links_count == 0 ||
> @@ -248,16 +252,19 @@ extents_loaded:
>  	memset(inode.i_block, 0, sizeof(inode.i_block));
>  
>  	/* Make a note of freed blocks */
> -	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
> +	quota_data_sub(ctx->qctx, &inode, ino,
> +		       list->blocks_freed * ctx->fs->blocksize);
> +	retval = ext2fs_iblk_sub_blocks(ctx->fs, inode_ptr, list->blocks_freed);
>  	if (retval)
>  		goto err;
>  
>  	/* Now stuff extents into the file */
> -	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
> +	retval = ext2fs_extent_open2(ctx->fs, ino, inode_ptr, &handle);
>  	if (retval)
>  		goto err;
>  
>  	ext_written = 0;
> +	start_val = ctx->clusters_allocated;
>  	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
>  		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
>  		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
> @@ -295,11 +302,17 @@ extents_loaded:
>  		ext_written++;
>  	}
>  
> +	if (start_val != ctx->clusters_allocated) {
> +		__u32 num = ctx->clusters_allocated - start_val;
> +		quota_data_add(ctx->qctx, &inode, ino,
> +			       num * ctx->fs->blocksize);
> +	}

Why not calculate the change in quota use by comparing the inode's
i_blocks in between the ext2fs_iblk_sub_blocks call and the end of the
ext2fs_extent_insert loop, rather than adding a counter to the global
e2fsck state?

--D

> +
>  #if defined(DEBUG) || defined(DEBUG_SUMMARY)
>  	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
>  	       ext_written);
>  #endif
> -	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
> +	e2fsck_write_inode(ctx, ino, inode_ptr, "rebuild_extents");
>  
>  err2:
>  	ext2fs_extent_free(handle);
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 188cc562..99e8f66a 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -4017,6 +4017,7 @@ static errcode_t e2fsck_get_alloc_block(ext2_filsys fs, blk64_t goal,
>  	}
>  
>  	*ret = new_block;
> +	ctx->clusters_allocated++;
>  	return (0);
>  }
>  
> -- 
> 2.11.0.rc0.7.gbe5a750
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled
  2017-04-15 13:41 ` [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled Theodore Ts'o
@ 2017-04-16 19:24   ` Darrick J. Wong
  2017-04-17  5:45     ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2017-04-16 19:24 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

On Sat, Apr 15, 2017 at 09:41:18AM -0400, Theodore Ts'o wrote:
> Add an extended option, -E no_optimize_extents, as well as a
> e2fsck.conf profile option, to disable extent tree optimization.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/e2fsck.8.in      | 4 ++++
>  e2fsck/e2fsck.conf.5.in | 4 ++++
>  e2fsck/e2fsck.h         | 1 +
>  e2fsck/extents.c        | 3 +++
>  e2fsck/unix.c           | 8 ++++++++
>  5 files changed, 20 insertions(+)
> 
> diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
> index 915273d8..4ad575f4 100644
> --- a/e2fsck/e2fsck.8.in
> +++ b/e2fsck/e2fsck.8.in
> @@ -226,6 +226,10 @@ option may prevent you from further manual data recovery.
>  Do not attempt to discard free blocks and unused inode blocks. This option is
>  exactly the opposite of discard option. This is set as default.
>  .TP
> +.BI no_optimize_extents
> +Do not offer to optimize the extent tree by eliminating unnecessary
> +width or depth.
> +.TP
>  .BI readahead_kb
>  Use this many KiB of memory to pre-fetch metadata in the hopes of reducing
>  e2fsck runtime.  By default, this is set to the size of two block groups' inode
> diff --git a/e2fsck/e2fsck.conf.5.in b/e2fsck/e2fsck.conf.5.in
> index 0bfc76ab..94525baf 100644
> --- a/e2fsck/e2fsck.conf.5.in
> +++ b/e2fsck/e2fsck.conf.5.in
> @@ -205,6 +205,10 @@ of that type are squelched.  This can be useful if the console is slow
>  (i.e., connected to a serial port) and so a large amount of output could
>  end up delaying the boot process for a long time (potentially hours).
>  .TP
> +.I no_optimize_extents
> +Do not offer to optimize the extent tree by eliminating unnecessary
> +width or depth.

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

/me wonders if there's a way to detect that we're being run by
initscripts/systemd/whatever on a device that is mounted ro, and skip
the optimization step to avoid the reboot.

OTOH I wonder why the initramfs (if available) doesn't just build in
e2fsck and fsck the root device before mounting the fs...

--D

> +.TP
>  .I readahead_mem_pct
>  Use this percentage of memory to try to read in metadata blocks ahead of the
>  main e2fsck thread.  This should reduce run times, depending on the speed of
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index ecd3b1e1..a4b5fb24 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -169,6 +169,7 @@ struct resource_track {
>  #define E2F_OPT_DISCARD		0x2000
>  #define E2F_OPT_CONVERT_BMAP	0x4000 /* convert blockmap to extent */
>  #define E2F_OPT_FIXES_ONLY	0x8000 /* skip all optimizations */
> +#define E2F_OPT_NOOPT_EXTENTS	0x10000 /* don't optimize extents */
>  
>  /*
>   * E2fsck flags
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index 1df31d71..d9410028 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -518,6 +518,9 @@ errcode_t e2fsck_should_rebuild_extents(e2fsck_t ctx,
>  	if (eti->force_rebuild)
>  		goto rebuild;
>  
> +	if (ctx->options & E2F_OPT_NOOPT_EXTENTS)
> +		return 0;
> +
>  	extents_per_block = (ctx->fs->blocksize -
>  			     sizeof(struct ext3_extent_header)) /
>  			    sizeof(struct ext3_extent);
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index b7322bc6..b6025535 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -709,6 +709,9 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
>  		} else if (strcmp(token, "nodiscard") == 0) {
>  			ctx->options &= ~E2F_OPT_DISCARD;
>  			continue;
> +		} else if (strcmp(token, "no_optimize_extents") == 0) {
> +			ctx->options |= E2F_OPT_NOOPT_EXTENTS;
> +			continue;
>  		} else if (strcmp(token, "log_filename") == 0) {
>  			if (!arg)
>  				extended_usage++;
> @@ -1007,6 +1010,11 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
>  	if (c)
>  		verbose = 1;
>  
> +	profile_get_boolean(ctx->profile, "options", "no_optimize_extents",
> +			    0, 0, &c);
> +	if (c)
> +		ctx->options |= E2F_OPT_NOOPT_EXTENTS;
> +
>  	if (ctx->readahead_kb == ~0ULL) {
>  		profile_get_integer(ctx->profile, "options",
>  				    "readahead_mem_pct", 0, -1, &c);
> -- 
> 2.11.0.rc0.7.gbe5a750
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled
  2017-04-16 19:24   ` Darrick J. Wong
@ 2017-04-17  5:45     ` Theodore Ts'o
  0 siblings, 0 replies; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-17  5:45 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Ext4 Developers List

On Sun, Apr 16, 2017 at 12:24:30PM -0700, Darrick J. Wong wrote:
> 
> /me wonders if there's a way to detect that we're being run by
> initscripts/systemd/whatever on a device that is mounted ro, and skip
> the optimization step to avoid the reboot.

That wasn't actually the reason for this option; but adding feature
where which does this for the root file system mounted read-only, the
optimization is avoided would make sense, yes.

> OTOH I wonder why the initramfs (if available) doesn't just build in
> e2fsck and fsck the root device before mounting the fs...

Debian does this already.  RHEL is just behind the times.  :-)

       	    	 	   	   	- Ted

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-16 19:17 ` [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Darrick J. Wong
@ 2017-04-21  7:37   ` Theodore Ts'o
  2017-04-21 16:27     ` Darrick J. Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-21  7:37 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Ext4 Developers List

On Sun, Apr 16, 2017 at 12:17:39PM -0700, Darrick J. Wong wrote:
> 
> Why not calculate the change in quota use by comparing the inode's
> i_blocks in between the ext2fs_iblk_sub_blocks call and the end of the
> ext2fs_extent_insert loop, rather than adding a counter to the global
> e2fsck state?

Good point.  That's a much simpler patch.  I now have:

>From 403bcb668e4f3d11baa5a503aff17fdaee5bf3c8 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Sat, 15 Apr 2017 00:29:46 -0400
Subject: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree

If quota is enabled, optimizing the extent tree wouldn't update the
in-memory quota statistics, so that a subsequent e2fsck run would show
that the quota usage statistics on disk were incorrect.

Google-Bug-Id: 36391645

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/e2fsck.c  |  1 +
 e2fsck/e2fsck.h  |  1 +
 e2fsck/extents.c | 23 ++++++++++++++++++-----
 e2fsck/pass1.c   |  1 +
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
index 0f9da46a..4baedbac 100644
--- a/e2fsck/e2fsck.c
+++ b/e2fsck/e2fsck.c
@@ -169,6 +169,7 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
 	ctx->fs_fragmented = 0;
 	ctx->fs_fragmented_dir = 0;
 	ctx->large_files = 0;
+	ctx->clusters_allocated = 0;
 
 	for (i=0; i < MAX_EXTENT_DEPTH_COUNT; i++)
 		ctx->extent_depth_count[i] = 0;
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index f3568106..ecd3b1e1 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -372,6 +372,7 @@ struct e2fsck_struct {
 	profile_t	profile;
 	int blocks_per_page;
 	ext2_u32_list encrypted_dirs;
+	__u32 clusters_allocated;
 
 	/* Reserve blocks for root and l+f re-creation */
 	blk64_t root_repair_block, lnf_repair_block;
diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index c4167e16..1df31d71 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -208,17 +208,21 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
 static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
 				     ext2_ino_t ino)
 {
-	struct ext2_inode	inode;
+	struct ext2_inode_large	inode;
+	struct ext2_inode	*inode_ptr;
 	errcode_t		retval;
 	ext2_extent_handle_t	handle;
 	unsigned int		i, ext_written;
 	struct ext2fs_extent	*ex, extent;
+	__u32			start_val;
 
 	list->count = 0;
 	list->blocks_freed = 0;
 	list->ino = ino;
 	list->ext_read = 0;
-	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
+	inode_ptr = (struct ext2_inode *) &inode;
+	e2fsck_read_inode_full(ctx, ino, inode_ptr, sizeof(inode),
+			       "rebuild_extents");
 
 	/* Skip deleted inodes and inline data files */
 	if (inode.i_links_count == 0 ||
@@ -248,16 +252,19 @@ extents_loaded:
 	memset(inode.i_block, 0, sizeof(inode.i_block));
 
 	/* Make a note of freed blocks */
-	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
+	quota_data_sub(ctx->qctx, &inode, ino,
+		       list->blocks_freed * ctx->fs->blocksize);
+	retval = ext2fs_iblk_sub_blocks(ctx->fs, inode_ptr, list->blocks_freed);
 	if (retval)
 		goto err;
 
 	/* Now stuff extents into the file */
-	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
+	retval = ext2fs_extent_open2(ctx->fs, ino, inode_ptr, &handle);
 	if (retval)
 		goto err;
 
 	ext_written = 0;
+	start_val = ctx->clusters_allocated;
 	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
 		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
 		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
@@ -295,11 +302,17 @@ extents_loaded:
 		ext_written++;
 	}
 
+	if (start_val != ctx->clusters_allocated) {
+		__u32 num = ctx->clusters_allocated - start_val;
+		quota_data_add(ctx->qctx, &inode, ino,
+			       num * ctx->fs->blocksize);
+	}
+
 #if defined(DEBUG) || defined(DEBUG_SUMMARY)
 	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
 	       ext_written);
 #endif
-	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
+	e2fsck_write_inode(ctx, ino, inode_ptr, "rebuild_extents");
 
 err2:
 	ext2fs_extent_free(handle);
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 188cc562..99e8f66a 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -4017,6 +4017,7 @@ static errcode_t e2fsck_get_alloc_block(ext2_filsys fs, blk64_t goal,
 	}
 
 	*ret = new_block;
+	ctx->clusters_allocated++;
 	return (0);
 }
 
-- 
2.11.0.rc0.7.gbe5a750

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-21  7:37   ` Theodore Ts'o
@ 2017-04-21 16:27     ` Darrick J. Wong
  2017-04-21 20:06       ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2017-04-21 16:27 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

On Fri, Apr 21, 2017 at 03:37:43AM -0400, Theodore Ts'o wrote:
> On Sun, Apr 16, 2017 at 12:17:39PM -0700, Darrick J. Wong wrote:
> > 
> > Why not calculate the change in quota use by comparing the inode's
> > i_blocks in between the ext2fs_iblk_sub_blocks call and the end of the
> > ext2fs_extent_insert loop, rather than adding a counter to the global
> > e2fsck state?
> 
> Good point.  That's a much simpler patch.  I now have:
> 
> From 403bcb668e4f3d11baa5a503aff17fdaee5bf3c8 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Sat, 15 Apr 2017 00:29:46 -0400
> Subject: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
> 
> If quota is enabled, optimizing the extent tree wouldn't update the
> in-memory quota statistics, so that a subsequent e2fsck run would show
> that the quota usage statistics on disk were incorrect.
> 
> Google-Bug-Id: 36391645
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  e2fsck/e2fsck.c  |  1 +
>  e2fsck/e2fsck.h  |  1 +
>  e2fsck/extents.c | 23 ++++++++++++++++++-----
>  e2fsck/pass1.c   |  1 +
>  4 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
> index 0f9da46a..4baedbac 100644
> --- a/e2fsck/e2fsck.c
> +++ b/e2fsck/e2fsck.c
> @@ -169,6 +169,7 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
>  	ctx->fs_fragmented = 0;
>  	ctx->fs_fragmented_dir = 0;
>  	ctx->large_files = 0;
> +	ctx->clusters_allocated = 0;

Uh... isn't this the same patch as before?

--D

>  
>  	for (i=0; i < MAX_EXTENT_DEPTH_COUNT; i++)
>  		ctx->extent_depth_count[i] = 0;
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index f3568106..ecd3b1e1 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -372,6 +372,7 @@ struct e2fsck_struct {
>  	profile_t	profile;
>  	int blocks_per_page;
>  	ext2_u32_list encrypted_dirs;
> +	__u32 clusters_allocated;
>  
>  	/* Reserve blocks for root and l+f re-creation */
>  	blk64_t root_repair_block, lnf_repair_block;
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index c4167e16..1df31d71 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -208,17 +208,21 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
>  static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
>  				     ext2_ino_t ino)
>  {
> -	struct ext2_inode	inode;
> +	struct ext2_inode_large	inode;
> +	struct ext2_inode	*inode_ptr;
>  	errcode_t		retval;
>  	ext2_extent_handle_t	handle;
>  	unsigned int		i, ext_written;
>  	struct ext2fs_extent	*ex, extent;
> +	__u32			start_val;
>  
>  	list->count = 0;
>  	list->blocks_freed = 0;
>  	list->ino = ino;
>  	list->ext_read = 0;
> -	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
> +	inode_ptr = (struct ext2_inode *) &inode;
> +	e2fsck_read_inode_full(ctx, ino, inode_ptr, sizeof(inode),
> +			       "rebuild_extents");
>  
>  	/* Skip deleted inodes and inline data files */
>  	if (inode.i_links_count == 0 ||
> @@ -248,16 +252,19 @@ extents_loaded:
>  	memset(inode.i_block, 0, sizeof(inode.i_block));
>  
>  	/* Make a note of freed blocks */
> -	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
> +	quota_data_sub(ctx->qctx, &inode, ino,
> +		       list->blocks_freed * ctx->fs->blocksize);
> +	retval = ext2fs_iblk_sub_blocks(ctx->fs, inode_ptr, list->blocks_freed);
>  	if (retval)
>  		goto err;
>  
>  	/* Now stuff extents into the file */
> -	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
> +	retval = ext2fs_extent_open2(ctx->fs, ino, inode_ptr, &handle);
>  	if (retval)
>  		goto err;
>  
>  	ext_written = 0;
> +	start_val = ctx->clusters_allocated;
>  	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
>  		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
>  		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
> @@ -295,11 +302,17 @@ extents_loaded:
>  		ext_written++;
>  	}
>  
> +	if (start_val != ctx->clusters_allocated) {
> +		__u32 num = ctx->clusters_allocated - start_val;
> +		quota_data_add(ctx->qctx, &inode, ino,
> +			       num * ctx->fs->blocksize);
> +	}
> +
>  #if defined(DEBUG) || defined(DEBUG_SUMMARY)
>  	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
>  	       ext_written);
>  #endif
> -	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
> +	e2fsck_write_inode(ctx, ino, inode_ptr, "rebuild_extents");
>  
>  err2:
>  	ext2fs_extent_free(handle);
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 188cc562..99e8f66a 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -4017,6 +4017,7 @@ static errcode_t e2fsck_get_alloc_block(ext2_filsys fs, blk64_t goal,
>  	}
>  
>  	*ret = new_block;
> +	ctx->clusters_allocated++;
>  	return (0);
>  }
>  
> -- 
> 2.11.0.rc0.7.gbe5a750
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-21 16:27     ` Darrick J. Wong
@ 2017-04-21 20:06       ` Theodore Ts'o
  2017-04-21 21:15         ` Andreas Dilger
  0 siblings, 1 reply; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-21 20:06 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Ext4 Developers List

Oops, Sorry.  Now with the corret version of the patch.

      	      	       	   	  - Ted

>From bc177d425d4fe33b0ae774b218b055465a840ca1 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Sat, 15 Apr 2017 00:29:46 -0400
Subject: [PATCH] e2fsck: update quota when optimizing the extent tree

If quota is enabled, optimizing the extent tree wouldn't update the
in-memory quota statistics, so that a subsequent e2fsck run would show
that the quota usage statistics on disk were incorrect.

Google-Bug-Id: 36391645

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 e2fsck/extents.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/e2fsck/extents.c b/e2fsck/extents.c
index c4167e16..7f28e6dd 100644
--- a/e2fsck/extents.c
+++ b/e2fsck/extents.c
@@ -208,17 +208,19 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
 static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
 				     ext2_ino_t ino)
 {
-	struct ext2_inode	inode;
+	struct ext2_inode_large	inode;
 	errcode_t		retval;
 	ext2_extent_handle_t	handle;
 	unsigned int		i, ext_written;
 	struct ext2fs_extent	*ex, extent;
+	blk64_t			start_val, delta;
 
 	list->count = 0;
 	list->blocks_freed = 0;
 	list->ino = ino;
 	list->ext_read = 0;
-	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
+	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode), sizeof(inode),
+			       "rebuild_extents");
 
 	/* Skip deleted inodes and inline data files */
 	if (inode.i_links_count == 0 ||
@@ -248,16 +250,20 @@ extents_loaded:
 	memset(inode.i_block, 0, sizeof(inode.i_block));
 
 	/* Make a note of freed blocks */
-	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
+	quota_data_sub(ctx->qctx, &inode, ino,
+		       list->blocks_freed * ctx->fs->blocksize);
+	retval = ext2fs_iblk_sub_blocks(ctx->fs, EXT2_INODE(&inode),
+					list->blocks_freed);
 	if (retval)
 		goto err;
 
 	/* Now stuff extents into the file */
-	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
+	retval = ext2fs_extent_open2(ctx->fs, ino, EXT2_INODE(&inode), &handle);
 	if (retval)
 		goto err;
 
 	ext_written = 0;
+	start_val = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode));
 	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
 		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
 		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
@@ -295,11 +301,21 @@ extents_loaded:
 		ext_written++;
 	}
 
+	delta = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode)) - start_val;
+	if (delta) {
+		if (!ext2fs_has_feature_huge_file(ctx->fs->super) ||
+		    !(inode.i_flags & EXT4_HUGE_FILE_FL))
+			delta <<= 9;
+		else
+			delta *= ctx->fs->blocksize;
+		quota_data_add(ctx->qctx, &inode, ino, delta);
+	}
+
 #if defined(DEBUG) || defined(DEBUG_SUMMARY)
 	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
 	       ext_written);
 #endif
-	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
+	e2fsck_write_inode(ctx, ino, EXT2_INODE(&inode), "rebuild_extents");
 
 err2:
 	ext2fs_extent_free(handle);
-- 
2.11.0.rc0.7.gbe5a750

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-21 20:06       ` Theodore Ts'o
@ 2017-04-21 21:15         ` Andreas Dilger
  2017-04-22 21:48           ` Theodore Ts'o
  0 siblings, 1 reply; 11+ messages in thread
From: Andreas Dilger @ 2017-04-21 21:15 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Darrick J. Wong, Ext4 Developers List

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

On Apr 21, 2017, at 2:06 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> From bc177d425d4fe33b0ae774b218b055465a840ca1 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Sat, 15 Apr 2017 00:29:46 -0400
> Subject: [PATCH] e2fsck: update quota when optimizing the extent tree
> 
> If quota is enabled, optimizing the extent tree wouldn't update the
> in-memory quota statistics, so that a subsequent e2fsck run would show
> that the quota usage statistics on disk were incorrect.
> 
> Google-Bug-Id: 36391645
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> e2fsck/extents.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/e2fsck/extents.c b/e2fsck/extents.c
> index c4167e16..7f28e6dd 100644
> --- a/e2fsck/extents.c
> +++ b/e2fsck/extents.c
> @@ -208,17 +208,19 @@ static int find_blocks(ext2_filsys fs, blk64_t *blocknr, e2_blkcnt_t blockcnt,
> static errcode_t rebuild_extent_tree(e2fsck_t ctx, struct extent_list *list,
> 				     ext2_ino_t ino)
> {
> -	struct ext2_inode	inode;
> +	struct ext2_inode_large	inode;
> 	errcode_t		retval;
> 	ext2_extent_handle_t	handle;
> 	unsigned int		i, ext_written;
> 	struct ext2fs_extent	*ex, extent;
> +	blk64_t			start_val, delta;
> 
> 	list->count = 0;
> 	list->blocks_freed = 0;
> 	list->ino = ino;
> 	list->ext_read = 0;
> -	e2fsck_read_inode(ctx, ino, &inode, "rebuild_extents");
> +	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode), sizeof(inode),
> +			       "rebuild_extents");

I'm always a bit puzzled why e2fsck_read_inode_full() takes ext2_inode
as an argument instead of ext2_inode_large, but that isn't the fault of
this patch.

> 
> 	/* Skip deleted inodes and inline data files */
> 	if (inode.i_links_count == 0 ||
> @@ -248,16 +250,20 @@ extents_loaded:
> 	memset(inode.i_block, 0, sizeof(inode.i_block));
> 
> 	/* Make a note of freed blocks */
> -	retval = ext2fs_iblk_sub_blocks(ctx->fs, &inode, list->blocks_freed);
> +	quota_data_sub(ctx->qctx, &inode, ino,
> +		       list->blocks_freed * ctx->fs->blocksize);

Should this quota_data_sub() call be after ext2fs_iblk_sub_blocks() has
completed successfully, or are the blocks already freed at this point
and only the inode->i_blocks counter would be outdated?

> +	retval = ext2fs_iblk_sub_blocks(ctx->fs, EXT2_INODE(&inode),
> +					list->blocks_freed);
> 	if (retval)
> 		goto err;
> 
> 	/* Now stuff extents into the file */
> -	retval = ext2fs_extent_open2(ctx->fs, ino, &inode, &handle);
> +	retval = ext2fs_extent_open2(ctx->fs, ino, EXT2_INODE(&inode), &handle);
> 	if (retval)
> 		goto err;
> 
> 	ext_written = 0;
> +	start_val = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode));

In theory this could go at the start before any changes are made to the
inode, and then there wouldn't be the need for the intermediate call to
quota_data_sub() above?  That avoids a small amount of overhead, and
also avoids the more complicated issue of the quota file being modified
even if this operation hits an error and is skipped (i.e. any "goto err"
case).

Cheers, Andreas

> 	for (i = 0, ex = list->extents; i < list->count; i++, ex++) {
> 		memcpy(&extent, ex, sizeof(struct ext2fs_extent));
> 		extent.e_flags &= EXT2_EXTENT_FLAGS_UNINIT;
> @@ -295,11 +301,21 @@ extents_loaded:
> 		ext_written++;
> 	}
> 
> +	delta = ext2fs_inode_i_blocks(ctx->fs, EXT2_INODE(&inode)) - start_val;
> +	if (delta) {
> +		if (!ext2fs_has_feature_huge_file(ctx->fs->super) ||
> +		    !(inode.i_flags & EXT4_HUGE_FILE_FL))
> +			delta <<= 9;
> +		else
> +			delta *= ctx->fs->blocksize;
> +		quota_data_add(ctx->qctx, &inode, ino, delta);
> +	}
> +
> #if defined(DEBUG) || defined(DEBUG_SUMMARY)
> 	printf("rebuild: ino=%d extents=%d->%d\n", ino, list->ext_read,
> 	       ext_written);
> #endif
> -	e2fsck_write_inode(ctx, ino, &inode, "rebuild_extents");
> +	e2fsck_write_inode(ctx, ino, EXT2_INODE(&inode), "rebuild_extents");
> 
> err2:
> 	ext2fs_extent_free(handle);
> --
> 2.11.0.rc0.7.gbe5a750
> 


Cheers, Andreas






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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] e2fsck: update quota when optimizing the extent tree
  2017-04-21 21:15         ` Andreas Dilger
@ 2017-04-22 21:48           ` Theodore Ts'o
  0 siblings, 0 replies; 11+ messages in thread
From: Theodore Ts'o @ 2017-04-22 21:48 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Darrick J. Wong, Ext4 Developers List

On Fri, Apr 21, 2017 at 03:15:07PM -0600, Andreas Dilger wrote:
> 
> Should this quota_data_sub() call be after ext2fs_iblk_sub_blocks() has
> completed successfully, or are the blocks already freed at this point
> and only the inode->i_blocks counter would be outdated?

The blocks are already freed at this point.

> In theory this could go at the start before any changes are made to the
> inode, and then there wouldn't be the need for the intermediate call to
> quota_data_sub() above?  That avoids a small amount of overhead, and
> also avoids the more complicated issue of the quota file being modified
> even if this operation hits an error and is skipped (i.e. any "goto err"
> case).

We're actually not modifying the quota file at this point; we're just
modifying the in-memory "correct" quota data numbers.

Also, if we fail in the middle, given that the extent tree blocks have
been freed and potentially partially reused, the fact that the quota
file has been modified is the least of our problems.  In theory we
could save the list of old extent tree index blocks, and try to use
new blocks for the new extent tree index blocks as much as possible.
But that's not true today.

    	   				- Ted
				

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-04-22 21:48 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-15 13:41 [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Theodore Ts'o
2017-04-15 13:41 ` [PATCH 2/3] tests: add new test f_quota_extent_opt Theodore Ts'o
2017-04-15 13:41 ` [PATCH 3/3] e2fsck: allow extent tree optimization to be disabled Theodore Ts'o
2017-04-16 19:24   ` Darrick J. Wong
2017-04-17  5:45     ` Theodore Ts'o
2017-04-16 19:17 ` [PATCH 1/3] e2fsck: update quota when optimizing the extent tree Darrick J. Wong
2017-04-21  7:37   ` Theodore Ts'o
2017-04-21 16:27     ` Darrick J. Wong
2017-04-21 20:06       ` Theodore Ts'o
2017-04-21 21:15         ` Andreas Dilger
2017-04-22 21:48           ` Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.