Thanks for very helpful reply, Jan: it changes everything, at the bottom. On Thu, 3 Aug 2023, Jan Kara wrote: > On Wed 02-08-23 17:00:49, Hugh Dickins wrote: > > > > This is certainly a problem, for both dquot_alloc and dquot_free paths. > > Thank you, Dan, for catching it. > > > > GFP_NOWAIT is an invitation to flakiness: I don't think it's right to > > regress existing quota users by changing GFP_NOFS to GFP_NOWAIT in all > > cases there; but it does seem a sensible stopgap for the new experimental > > user tmpfs. > > So passing gfp argument to quota_send_warning() and propagating the > blocking info through __dquot_alloc_space() and __dquot_free_space() flags > would be OK for me *but* if CONFIG_PRINT_QUOTA_WARNING is set (which is > deprecated but still exists), we end up calling tty_write_message() out of > flush_warnings() and that can definitely block. Oh yes :( > > So if we are looking for unintrusive stopgap solution, maybe tmpfs can just > tell quota code to not issue warnings at all by using > __dquot_alloc_space() without DQUOT_SPACE_WARN flag and add support for > this flag to __dquot_free_space()? The feature is not used too much AFAIK > anyway. And once we move dquot calls into places where they can sleep, we > can reenable the warning support. If the warning feature is not used very much at all (I did not realize that), then certainly this would be a better way to go for now, than the inadequate and extra DQUOT_SPACE_WARN_NOWAIT I was suggesting. > > > I think the thing to do, for now, is to add a flag (DQUOT_SPACE_WARN_NOWAIT?) > > which gets passed down to the __dquot_alloc and __dquot_free for tmpfs, > > and those choose GFP_NOFS or GFP_NOWAIT accordingly, and pass that gfp_t > > on down to flush_warnings() to quota_send_warning() to genlmsg_new() and > > genlmsg_multicast(). Carlos, if you agree, please try that. Carlos, sorry, please don't waste your time on DQUOT_SPACE_WARN_NOWAIT or no-DQUOT_SPACE_WARN. > > > > I have no experience with netlink whatsoever: I hope that will be enough > > to stop it from blocking. > > Yes, if you pass non-blocking gfp mode to netlink code, it takes care not > to block when allocating and sending the message. Useful info, thanks. > > > I did toy with the idea of passing back the dquot_warn, and letting the > > caller do the flush_warnings() at a more suitable moment; and that might > > work out, but I suspect that the rearrangement involved would be better > > directed to just rearranging where mm/shmem.c makes it dquot_alloc and > > dquot_free calls. > > Yeah, frankly I think this is the best fix. AFAIU the problem is only with > shmem_recalc_inode() getting called under info->lock which looks managable > as far as I'm looking at the call sites and relatively easy wrt quotas as > freeing of quota space cannot fail. At least all shmem_inode_acct_blocks() > calls seem to be in places where they can sleep. Ah, I believe you're right, and that's great: I was living in the past, when shmem_charge() was still calling shmem_inode_acct_block() under info->lock. I agree, the only problem appears to be that shmem_inode_unacct_blocks() call which I had to place inside shmem_recalc_inode(): under info->lock, so I was just perpetuating the problems - extending them even. So the fix should not require any rearrangement of where the dquot_alloc and dquot_free are done: I may want to do so later, when updating to fix the failures of concurrent allocation of last block, but there's no need to get into any such rearrangement as part of this fix. We just want shmem_recalc_inode() to take the info->lock itself, do its adjustments and balancing, release the lock and dquot_free the excess. I had a quick look through that, most places look straightforward to update, but there are a couple where I need to think a bit first. So no patch in this mail, but I'll get back to it in a few hours. Hugh
