linux-ext4 - Re: [PATCH 1/1] quota: flush quota_release

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b2ea547a-6097-4f95-9ee7-097c8363a076@huawei.com>
Date: Tue, 19 Nov 2024 14:29:57 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: Ojaswin Mujoo <ojaswin@...ux.ibm.com>, Ritesh Harjani
	<ritesh.list@...il.com>, <linux-ext4@...r.kernel.org>, Jan Kara
	<jack@...e.com>, <linux-kernel@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>, Disha Goel <disgoel@...ux.ibm.com>, Yang
 Erkun <yangerkun@...wei.com>
Subject: Re: [PATCH 1/1] quota: flush quota_release_work upon quota writeback

On 2024/11/18 20:53, Jan Kara wrote:
> On Mon 18-11-24 09:29:19, Baokun Li wrote:
>> On 2024/11/17 1:59, Ojaswin Mujoo wrote:
>>> On Sat, Nov 16, 2024 at 02:20:26AM +0530, Ritesh Harjani wrote:
>>>> Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:
>>>>
>>>>> One of the paths quota writeback is called from is:
>>>>>
>>>>> freeze_super()
>>>>>     sync_filesystem()
>>>>>       ext4_sync_fs()
>>>>>         dquot_writeback_dquots()
>>>>>
>>>>> Since we currently don't always flush the quota_release_work queue in
>>>>> this path, we can end up with the following race:
>>>>>
>>>>>    1. dquot are added to releasing_dquots list during regular operations.
>>>>>    2. FS freeze starts, however, this does not flush the quota_release_work queue.
>>>>>    3. Freeze completes.
>>>>>    4. Kernel eventually tries to flush the workqueue while FS is frozen which
>>>>>       hits a WARN_ON since transaction gets started during frozen state:
>>>>>
>>>>>     ext4_journal_check_start+0x28/0x110 [ext4] (unreliable)
>>>>>     __ext4_journal_start_sb+0x64/0x1c0 [ext4]
>>>>>     ext4_release_dquot+0x90/0x1d0 [ext4]
>>>>>     quota_release_workfn+0x43c/0x4d0
>>>>>
>>>>> Which is the following line:
>>>>>
>>>>>     WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE);
>>>>>
>>>>> Which ultimately results in generic/390 failing due to dmesg
>>>>> noise. This was detected on powerpc machine 15 cores.
>>>>>
>>>>> To avoid this, make sure to flush the workqueue during
>>>>> dquot_writeback_dquots() so we dont have any pending workitems after
>>>>> freeze.
>>>> Not just that, sync_filesystem can also be called from other places and
>>>> quota_release_workfn() could write out and and release the dquot
>>>> structures if such are found during processing of releasing_dquots list.
>>>> IIUC, this was earlier done in the same dqput() context but had races
>>>> with dquot_mark_dquot_dirty(). Hence the final dqput() will now add the
>>>> dquot structures to releasing_dquots list and will schedule a delayed
>>>> workfn which will process the releasing_dquots list.
>>> Hi Ritesh,
>>>
>>> Ohh right, thanks for the context. I see this was done here:
>>>
>>>     dabc8b207566 quota: fix dqput() to follow the guarantees dquot_srcu
>>>     should provide
> Yup.
>
>> Nice catch! Thanks for fixing this up!
>>
>> Have you tested the performance impact of this patch? It looks like the
>> unconditional call to flush_delayed_work() in dquot_writeback_dquots()
>> may have some performance impact for frequent chown/sync scenarios.
> Well, but sync(2) or so is expensive anyway. Also dquot_writeback_dquots()
> should persist all pending quota modifications and it is true that pending
> dquot_release() calls can remove quota structures from the quota file and
> thus are by definition pending modifications. So I agree with Ojaswin that
> putting the workqueue flush there makes sense and is practically required
> for data consistency guarantees.
Make sense.
>> When calling release_dquot(), we will only remove the quota of an object
>> (user/group/project) from disk if it is not quota-limited and does not
>> use any inode or block.
>>
>> Asynchronous removal is now much more performance friendly, not only does
>> it make full use of the multi-core, but for scenarios where we have to
>> repeatedly chown between two objects, delayed release avoids the need to
>> repeatedly allocate/free space in memory and on disk.
> True, but unless you call sync(2) in between these two calls this is going
> to still hold.
Yeah without sync or syncfs, it's the same as before.
>> Overall, since the actual dirty data is already on the disk, there is no
>> consistency issue here as it is just clearing unreferenced quota on the
>> disk, so I thought maybe it would be better to call flush_delayed_work()
>> in the freeze context.
> To summarise, I don't think real-life workloads are going to observe the
> benefit and conceptually the call really belongs more to
> dquot_writeback_dquots().
>
> 								Honza

Okay, thanks for the feedback!


Regards,
Baokun