From: Jan Kara <jack@suse.cz>
To: yangerkun <yangerkun@huawei.com>
Cc: Jan Kara <jack@suse.cz>, Theodore Ts'o <tytso@mit.edu>,
Ext4 Developers List <linux-ext4@vger.kernel.org>,
yukuai3@huawei.com
Subject: Re: [PATCH] ext4: if zeroout fails fall back to splitting the extent node
Date: Wed, 24 Nov 2021 18:15:10 +0100 [thread overview]
Message-ID: <20211124171510.GA11240@quack2.suse.cz> (raw)
In-Reply-To: <9e47b349-0360-3426-dfa3-cc77f444fac3@huawei.com>
On Wed 24-11-21 20:11:43, yangerkun wrote:
>
>
> On 2021/11/24 18:37, Jan Kara wrote:
> > On Wed 24-11-21 17:01:12, yangerkun wrote:
> > > On 2021/11/23 17:27, Jan Kara wrote:
> > > > Hello,
> > > >
> > > > On Sun 26-09-21 19:35:01, yangerkun wrote:
> > > > > Rethink about this problem. Should we consider other place which call
> > > > > ext4_issue_zeroout? Maybe it can trigger the problem too(in theory, not
> > > > > really happened)...
> > > > >
> > > > > How about include follow patch which not only transfer ENOSPC to EIO. But
> > > > > also stop to overwrite the error return by ext4_ext_insert_extent in
> > > > > ext4_split_extent_at.
> > > > >
> > > > > Besides, 308c57ccf431 ("ext4: if zeroout fails fall back to splitting the
> > > > > extent node") can work together with this patch.
> > > >
> > > > I've got back to this. The ext4_ext_zeroout() calls in
> > > > ext4_split_extent_at() seem to be there as fallback when insertion of a new
> > > > extent fails due to ENOSPC / EDQUOT. If even ext4_ext_zeroout(), then I
> > > > think returning an error as the code does now is correct and we don't have
> > > > much other option. Also we are really running out of disk space so I think
> > > > returning ENOSPC is fine. What exact scenario are you afraid of?
> > >
> > > I am afraid about the EDQUOT from ext4_ext_insert_extent may be overwrite by
> > > ext4_ext_zeroout with ENOSPC. And this may lead to dead loop since
> > > ext4_writepages will retry once get ENOSPC? Maybe I am wrong...
> >
> > OK, so passing back original error instead of the error from
> > ext4_ext_zeroout() makes sense. But I don't think doing much more is needed
> > - firstly, ENOSPC or EDQUOT should not happen in ext4_split_extent_at()
> > called from ext4_writepages() because we should have reserved enough
> > space for extent splits when writing data. So hitting that is already
>
> ext4_da_write_begin
> ext4_da_get_block_prep
> ext4_insert_delayed_block
> ext4_da_reserve_space
>
> It seems we will only reserve space for data, no for metadata...
>
>
> > unexpected. Committing transaction holding blocks that are expected to be
> > free is the most likely reason for us seeing ENOSPC and returning EIO in
> > that case would be bug.
>
> Agree. EIO from ext4_ext_zeroout that overwrite the ENOSPC from
> ext4_ext_insert_extent seems buggy too. Maybe we should ignore the error
> from ext4_ext_zeroout and return the error from ext4_ext_insert_extent
> once ext4_ext_zeroout in ext4_split_extent_at got a error. Something
> like this:
Yep, something like that looks good to me.
Honza
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0ecf819bf189..56cc00ee42a1 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3185,6 +3185,7 @@ static int ext4_split_extent_at(handle_t *handle,
> struct ext4_extent *ex2 = NULL;
> unsigned int ee_len, depth;
> int err = 0;
> + int err1;
>
> BUG_ON((split_flag & (EXT4_EXT_DATA_VALID1 | EXT4_EXT_DATA_VALID2))
> ==
> (EXT4_EXT_DATA_VALID1 | EXT4_EXT_DATA_VALID2));
> @@ -3255,7 +3256,7 @@ static int ext4_split_extent_at(handle_t *handle,
> if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> if (split_flag &
> (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
> if (split_flag & EXT4_EXT_DATA_VALID1) {
> - err = ext4_ext_zeroout(inode, ex2);
> + err1 = ext4_ext_zeroout(inode, ex2);
> zero_ex.ee_block = ex2->ee_block;
> zero_ex.ee_len = cpu_to_le16(
>
> ext4_ext_get_actual_len(ex2));
> @@ -3270,7 +3271,7 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_pblock(ex));
> }
> } else {
> - err = ext4_ext_zeroout(inode, &orig_ex);
> + err1 = ext4_ext_zeroout(inode, &orig_ex);
> zero_ex.ee_block = orig_ex.ee_block;
> zero_ex.ee_len = cpu_to_le16(
>
> ext4_ext_get_actual_len(&orig_ex));
> @@ -3278,7 +3279,7 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_pblock(&orig_ex));
> }
>
> - if (!err) {
> + if (!err1) {
> /* update the extent length and mark as initialized
> */
> ex->ee_len = cpu_to_le16(ee_len);
> ext4_ext_try_to_merge(handle, inode, path, ex);
>
>
>
> > Secondly, returning EIO instead of ENOSPC is IMO a
> > bit confusing for upper layers and makes it harder to analyze where the
> > real problem is...
> >
> > Honza
> >
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2021-11-24 17:15 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-04 12:50 [PATCH] ext4: stop return ENOSPC from ext4_issue_zeroout yangerkun
2021-08-04 13:35 ` Jan Kara
2021-08-13 15:18 ` Theodore Ts'o
2021-08-13 21:27 ` [PATCH] ext4: if zeroout fails fall back to splitting the extent node Theodore Ts'o
2021-08-14 2:15 ` yangerkun
2021-08-16 6:57 ` yangerkun
2021-09-26 11:35 ` yangerkun
2021-11-23 9:27 ` Jan Kara
2021-11-24 9:01 ` yangerkun
2021-11-24 10:37 ` Jan Kara
2021-11-24 12:11 ` yangerkun
2021-11-24 17:15 ` Jan Kara [this message]
2021-08-16 10:05 ` [PATCH] ext4: stop return ENOSPC from ext4_issue_zeroout Jan Kara
2021-08-16 14:16 ` Theodore Ts'o
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211124171510.GA11240@quack2.suse.cz \
--to=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yangerkun@huawei.com \
--cc=yukuai3@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).