Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230324092907.1341457-1-cccheng@synology.com>
 <20230327092914.mzizhh52izbvjhhv@quack3> <CAHuHWt=LaNBwNy-1RY2-OZ4zGKEgTBfZZGWoQJSjL3ADbRRCoQ@mail.gmail.com>
 <08c67287-53d8-58ce-e4bb-b1656bc6013e@huawei.com>
In-Reply-To: <08c67287-53d8-58ce-e4bb-b1656bc6013e@huawei.com>
From:   Chung-Chiang Cheng <shepjeng@gmail.com>
Date:   Wed, 29 Mar 2023 11:36:46 +0800
Message-ID: <CAHuHWtnPtMscpO+fxYncQ_K+T+j2AGxG75kimoL6gp5Q__GfTw@mail.gmail.com>
Subject: Re: [PATCH] ext4: defer updating i_disksize until endio
To:     Zhang Yi <yi.zhang@huawei.com>, Jan Kara <jack@suse.cz>
Cc:     Chung-Chiang Cheng <cccheng@synology.com>,
        linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, kernel@cccheng.net,
        Robbie Ko <robbieko@synology.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 27, 2023 at 7:17=E2=80=AFPM Zhang Yi <yi.zhang@huawei.com> wrot=
e:
>
> On 2023/3/27 18:28, Chung-Chiang Cheng wrote:
> > It's a pity that this issue also occurs with data=3Dordered due to dela=
yed
> > allocation being enabled by default. If delayed allocation were disable=
d,
> > it would not be as easy to reproduce.
> >
> > This is because if data is written to the end of a file and the block i=
s
> > allocated, the new i_disksize will be immediately committed to the jour=
nal
> > at ext4_da_write_end(), but the writeback procedure is not yet triggere=
d.
> > By default, ext4 commits the journal every 5 seconds, but a dirty page =
may
> > not be written back until 30 seconds later. This is not a short time wi=
ndow,
> > and any improper shutdown during this time may lead to the issue :(
> >

Thank you for the explanation from you and Jan. I agree that it is not the
responsibility of ext4 to provide application consistency, but this case is
not even crash consistent, although no sensitive data were revealed after
crash.

> It seems that the case you've mentioned is intra-block append write (no?)=
,
> current data=3Dordered mount option doesn't work in this case because
> ext4_map_blocks() doesn't attach inode to the t_inode_list of the running
> transaction. If delayed allocation were disabled, the lose data window is=
 still
> there, because ext4_write_end()->ext4_update_inode_size() is also updatin=
g
> i_disksize before writing data back. This is at least guarantee no store =
data.
> We had discussed this in [1].

Yes, you're right. I've reconfirmed my experiment and determined that this
issue can be reproduced with or without delayed allocation.

I've tried your previous solution of adding the required inode to the curre=
nt
transaction's ordered data list. It seems to work perfectly for me and simp=
ly
solves the issue, but the journal handling needs to be added back to the
delayed allocation write. Does it really have an obvious performance impact=
?

>
> [1]. https://lore.kernel.org/linux-ext4/1554370192-113254-1-git-send-emai=
l-yi.zhang@huawei.com/
>
> Thanks,
> Yi.