Re: [PATCH] shmem: Serialize fallocate progress with writeout

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 11/11/26 4:41 PM, Zhu Jun wrote:
> shmem_writeout() uses inode->i_lock when reading fallocate progress and
> updating nr_unswapped, but shmem_fallocate() accesses the shared progress
> without that lock. Holding the current folio lock does not exclude
> writeout of a different folio, as the KCSAN report demonstrates.

Could you show the KCSAN report?

Also, what's the actual impact? It looks like there's no significant 
effect to me. For this kind of concurrency, I think READ_ONCE/WRITE_ONCE 
should be sufficient?

> Use inode->i_lock to sample the unswapped count and publish fallocate
> progress. This matches the writeout side and keeps the existing folio
> to inode lock ordering.
> 
> Fixes: 1aac1400319d ("tmpfs: quit when fallocate fills memory")
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
> ---
>   mm/shmem.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 990f1b083779..5769cfab1d71 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3969,6 +3969,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   
>   	for (index = start; index < end; ) {
>   		struct folio *folio;
> +		bool unswapped;
> +
> +		spin_lock(&inode->i_lock);
> +		unswapped = shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced;
> +		spin_unlock(&inode->i_lock);
>   
>   		/*
>   		 * Check for fatal signal so that we abort early in OOM
> @@ -3979,7 +3984,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   		 */
>   		if (fatal_signal_pending(current))
>   			error = -EINTR;
> -		else if (shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced)
> +		else if (unswapped)
>   			error = -ENOMEM;
>   		else
>   			error = shmem_get_folio(inode, index, offset + len,
> @@ -4007,11 +4012,13 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   
>   		/*
>   		 * Inform shmem_writeout() how far we have reached.
> -		 * No need for lock or barrier: we have the page lock.
> +		 * Other folios can be written out while this folio is locked.
>   		 */
> +		spin_lock(&inode->i_lock);
>   		if (!folio_test_uptodate(folio))
>   			shmem_falloc.nr_falloced += index - shmem_falloc.next;
>   		shmem_falloc.next = index;
> +		spin_unlock(&inode->i_lock);
>   
>   		/*
>   		 * If !uptodate, leave it that way so that freeable folios