lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 24 Nov 2021 17:01:12 +0800 From: yangerkun <yangerkun@...wei.com> To: Jan Kara <jack@...e.cz> CC: Theodore Ts'o <tytso@....edu>, Ext4 Developers List <linux-ext4@...r.kernel.org>, <yukuai3@...wei.com> Subject: Re: [PATCH] ext4: if zeroout fails fall back to splitting the extent node 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... > > Honza > >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index c0de30f25185..66767ede235f 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -3218,16 +3218,18 @@ static int ext4_split_extent_at(handle_t *handle, >> goto out; >> >> if (EXT4_EXT_MAY_ZEROOUT & split_flag) { >> + int ret = 0; >> + >> if (split_flag & >> (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) { >> if (split_flag & EXT4_EXT_DATA_VALID1) { >> - err = ext4_ext_zeroout(inode, ex2); >> + ret = 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)); >> ext4_ext_store_pblock(&zero_ex, >> >> ext4_ext_pblock(ex2)); >> } else { >> - err = ext4_ext_zeroout(inode, ex); >> + ret = ext4_ext_zeroout(inode, ex); >> zero_ex.ee_block = ex->ee_block; >> zero_ex.ee_len = cpu_to_le16( >> >> ext4_ext_get_actual_len(ex)); >> @@ -3235,7 +3237,7 @@ static int ext4_split_extent_at(handle_t *handle, >> ext4_ext_pblock(ex)); >> } >> } else { >> - err = ext4_ext_zeroout(inode, &orig_ex); >> + ret = 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)); >> @@ -3243,7 +3245,7 @@ static int ext4_split_extent_at(handle_t *handle, >> ext4_ext_pblock(&orig_ex)); >> } >> >> - if (!err) { >> + if (!ret) { >> /* 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); >> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c >> index d18852d6029c..95b970581864 100644 >> --- a/fs/ext4/inode.c >> +++ b/fs/ext4/inode.c >> @@ -427,6 +427,9 @@ int ext4_issue_zeroout(struct inode *inode, ext4_lblk_t >> lblk, ext4_fsblk_t pblk, >> if (ret > 0) >> ret = 0; >> >> + if (ret == -ENOSPC) >> + ret = -EIO; >> + >> return ret; >> } >> >> >> >> 在 2021/8/14 5:27, Theodore Ts'o 写道: >>> If the underlying storage device is using thin-provisioning, it's >>> possible for a zeroout operation to return ENOSPC. >>> >>> Commit df22291ff0fd ("ext4: Retry block allocation if we have free blocks >>> left") added logic to retry block allocation since we might get free block >>> after we commit a transaction. But the ENOSPC from thin-provisioning >>> will confuse ext4, and lead to an infinite loop. >>> >>> Since using zeroout instead of splitting the extent node is an >>> optimization, if it fails, we might as well fall back to splitting the >>> extent node. >>> >>> Reported-by: yangerkun <yangerkun@...wei.com> >>> Signed-off-by: Theodore Ts'o <tytso@....edu> >>> --- >>> >>> I've run this through my battery of tests, and it doesn't cause any >>> regressions. Yangerkun, can you test this and see if this works for >>> you? >>> >>> fs/ext4/extents.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >>> index 92ad64b89d9b..501516cadc1b 100644 >>> --- a/fs/ext4/extents.c >>> +++ b/fs/ext4/extents.c >>> @@ -3569,7 +3569,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, >>> split_map.m_len - ee_block); >>> err = ext4_ext_zeroout(inode, &zero_ex1); >>> if (err) >>> - goto out; >>> + goto fallback; >>> split_map.m_len = allocated; >>> } >>> if (split_map.m_lblk - ee_block + split_map.m_len < >>> @@ -3583,7 +3583,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, >>> ext4_ext_pblock(ex)); >>> err = ext4_ext_zeroout(inode, &zero_ex2); >>> if (err) >>> - goto out; >>> + goto fallback; >>> } >>> split_map.m_len += split_map.m_lblk - ee_block; >>> @@ -3592,6 +3592,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, >>> } >>> } >>> +fallback: >>> err = ext4_split_extent(handle, inode, ppath, &split_map, split_flag, >>> flags); >>> if (err > 0) >>>
Powered by blists - more mailing lists