On Wed 02-08-23 17:00:49, Hugh Dickins wrote: > On Wed, 2 Aug 2023, Carlos Maiolino wrote: > > On Wed, Aug 02, 2023 at 09:53:54AM +0300, Dan Carpenter wrote: > > > Hello Carlos Maiolino, > > > > > > The patch 9a9f8f590f6d: "shmem: quota support" from Jul 25, 2023 > > > (linux-next), leads to the following Smatch static checker warning: > > > > > > fs/quota/dquot.c:1271 flush_warnings() > > > warn: sleeping in atomic context > > > > > > > Thanks for the report Dan! > > > > > fs/quota/dquot.c > > > 1261 static void flush_warnings(struct dquot_warn *warn) > > > 1262 { > > > 1263 int i; > > > 1264 > > > 1265 for (i = 0; i < MAXQUOTAS; i++) { > > > 1266 if (warn[i].w_type == QUOTA_NL_NOWARN) > > > 1267 continue; > > > 1268 #ifdef CONFIG_PRINT_QUOTA_WARNING > > > 1269 print_warning(&warn[i]); > > > 1270 #endif > > > --> 1271 quota_send_warning(warn[i].w_dq_id, > > > 1272 warn[i].w_sb->s_dev, warn[i].w_type); > > > > > > The quota_send_warning() function does GFP_NOFS allocations, which don't > > > touch the fs but can still sleep. GFP_ATOMIC or GFP_NOWAIT don't sleep. > > > > > > > Hmm, tbh I think the simplest way to fix it is indeed change GFP_NOFS to > > GFP_NOWAIT when calling genlmsg_new(), quota_send_warnings() already abstain to > > pass any error back to its caller, I don't think moving it from GFP_NOFS -> > > GFP_NOWAIT will have much impact here as the amount of memory required for it is > > not that big and wouldn't fail unless free memory is really short. I might be > > wrong though. > > > > If not that, another option would be to swap tmpfs spinlocks for mutexes, but I > > would rather avoid that. > > > > CC'ing other folks for more suggestions. > > 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. 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. > 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. > > 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. > 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. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR