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@xxxxxxxxxxxxx> 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. > 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. > 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 -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
