Re: [bug report] shmem: quota support

[Jan Kara <jack@suse.cz>]

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@suse.com>
SUSE Labs, CR