Re: [bug report] shmem: quota support

[Carlos Maiolino <cem@kernel.org>]

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.

Carlos

>     1273         }
>     1274 }
> 
> The call trees that Smatch is worried about are listed.  The "disables
> preempt" functions take the spin_lock_irq(&info->lock) before calling
> shmem_recalc_inode().
> 
> shmem_charge() <- disables preempt
> shmem_uncharge() <- disables preempt
> shmem_undo_range() <- disables preempt
> shmem_getattr() <- disables preempt
> shmem_writepage() <- disables preempt
> shmem_set_folio_swapin_error() <- disables preempt
> shmem_swapin_folio() <- disables preempt
> shmem_get_folio_gfp() <- disables preempt
> -> shmem_recalc_inode()
>    -> shmem_inode_unacct_blocks()
>       -> dquot_free_block_nodirty()
>          -> dquot_free_space_nodirty()
>             -> __dquot_free_space()
>                -> flush_warnings()

Hm, I see, we add dquot_free_block_nodirty() call to shmem_inode_unacct_blocks()
here, which leads to this possible sleep inside spin_lock_irq().
> 
> regards,
> dan carpenter