by Guoqing Jiang

[permalink] [raw]

Subject: Re: [PATCH V3 5/5] ext4: make fallocate retry when err is ENOSPC

Hi,

On 7/24/21 3:41 PM, Wang Jianchao wrote:
> From: Wang Jianchao <[email protected]>
>
> The blocks may be waiting for journal commit to be freed back to
> mb buddy. Let fallocate wait and retry in that case.
>
> Signed-off-by: Wang Jianchao <[email protected]>
> ---
> fs/ext4/extents.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 92ad64b89d9b..ad0b874d3448 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4635,7 +4635,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> struct inode *inode = file_inode(file);
> loff_t new_size = 0;
> unsigned int max_blocks;
> - int ret = 0;
> + int ret = 0, retries = 0;
> int flags;
> ext4_lblk_t lblk;
> unsigned int blkbits = inode->i_blkbits;
> @@ -4656,6 +4656,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> FALLOC_FL_INSERT_RANGE))
> return -EOPNOTSUPP;
>
> +retry:
> ext4_fc_start_update(inode);
>
> if (mode & FALLOC_FL_PUNCH_HOLE) {
> @@ -4722,6 +4723,9 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> trace_ext4_fallocate_exit(inode, offset, max_blocks, ret);
> exit:
> ext4_fc_stop_update(inode);
> + if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
> + goto retry;
> +

Not sure if it is necessary since ext4_alloc_file_blocks already retries
allocate.

Thanks,
Guoqing

2021-07-26 03:42:47

On 2021/8/4 11:52 PM, Jan Kara wrote:
> On Mon 26-07-21 15:05:41, Wang Jianchao wrote:
>>
>>
>> On 2021/7/26 11:40 AM, Guoqing Jiang wrote:
>>> Hi,
>>>
>>> On 7/24/21 3:41 PM, Wang Jianchao wrote:
>>>> From: Wang Jianchao <[email protected]>
>>>>
>>>> The blocks may be waiting for journal commit to be freed back to
>>>> mb buddy. Let fallocate wait and retry in that case.
>>>>
>>>> Signed-off-by: Wang Jianchao <[email protected]>
>>>> ---
>>>> fs/ext4/extents.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>>> index 92ad64b89d9b..ad0b874d3448 100644
>>>> --- a/fs/ext4/extents.c
>>>> +++ b/fs/ext4/extents.c
>>>> @@ -4635,7 +4635,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>>>       struct inode *inode = file_inode(file);
>>>>       loff_t new_size = 0;
>>>>       unsigned int max_blocks;
>>>> -    int ret = 0;
>>>> +    int ret = 0, retries = 0;
>>>>       int flags;
>>>>       ext4_lblk_t lblk;
>>>>       unsigned int blkbits = inode->i_blkbits;
>>>> @@ -4656,6 +4656,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>>>                FALLOC_FL_INSERT_RANGE))
>>>>           return -EOPNOTSUPP;
>>>> +retry:
>>>>       ext4_fc_start_update(inode);
>>>>       if (mode & FALLOC_FL_PUNCH_HOLE) {
>>>> @@ -4722,6 +4723,9 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>>>       trace_ext4_fallocate_exit(inode, offset, max_blocks, ret);
>>>> exit:
>>>>       ext4_fc_stop_update(inode);
>>>> +    if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>>>> +        goto retry;
>>>> +
>>>
>>> Not sure if it is necessary since ext4_alloc_file_blocks already retries allocate.
>>
>> Yes, this patch should be get rid of. But it is indeed helpful to fix
>> the xfstest generic/371 which does concurrently write/rm and
>> fallocate/rm. I'll figure out some other way to improve that
>
> Note that the retry logic is only a heuristic. It is not guaranteed any
> number of retries is enough, we just do three to not give up too easily...
> Your patch effectively raised number of retries to 9 so that may have
> masked the issue. But I don't think so high number of retries is a sensible
> choice because that way it may take too long to return ENOSPC.

The failure seems due to the background discard which marks the blocks used
before issue discard.

The test make a 256M filesystem which has 59316 4K blocks.
There are two thread running concurrently,
- write, rm 80M file
- fallocate, rm 80M file

When the fallocate failed, I can observe there was a 80M on-going background trim

We seems to need to add a flush_work(sbi->s_discard_work) into ext4_should_retry_alloc()

Thanks so much
Jianchao
>
> Honza
>