Re: [PATCH v3 1/3] mm/truncate: fix data loss when splitting straddling large folios fails

[Jan Kara]

On Thu 17-09-26 20:03:57, Zhang Yi wrote:
> Sorry, the previous diff format was incorrect, reposting it.
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 23c90f00b530..b5e5bdf5f9ef 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -279,51 +279,46 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
>  			*pstart = round_up(pos + offset,
>  					   min_nrbytes) >> PAGE_SHIFT;
> 
> -		if (offset + length == size) {
> -			end = aligned_end;
> +		end = aligned_end;
> +		if (offset + length == size)
>  			goto out;
> -		}
> -
> -		split_at2 = folio_page(folio,
> -				PAGE_ALIGN_DOWN(offset + length) / PAGE_SIZE);
> -		folio2 = page_folio(split_at2);
> 
>  		/*
> -		 * folio2 may become stale due to a concurrent split or
> -		 * freeing, so validate it before and after taking its lock.
> -		 * If it fails, we can't get an accurate end position and fall
> -		 * back to folio->index, which may leave sub-folios split off
> -		 * at the offset edge in the page cache this round.
> +		 * After the first split at the start edge, the folio at the
> +		 * end edge may be freed and reused concurrently.
> +		 * __filemap_get_folio() looks up the straddler at
> +		 * aligned_end and returns it locked and ref'd with the
> +		 * mapping validated.
>  		 */
> -		end = folio->index;
> -		if (!folio_try_get(folio2))
> +		folio2 = __filemap_get_folio(folio->mapping, aligned_end,
> +					     FGP_LOCK | FGP_NOWAIT, 0);
> +		if (IS_ERR(folio2)) {
> +			/*
> +			 * No sub-folio straddles the boundary when aligned_end
> +			 * is empty so discarding up to it is safe.  Otherwise
> +			 * the straddler is locked by someone else and we
> +			 * cannot obtain a reliable end position, so we fall
> +			 * back to folio->index.  This is safe but may leave
> +			 * pages split off at the offset edge lingering in the
> +			 * page cache this round.
> +			 */
> +			if (PTR_ERR(folio2) != -ENOENT)
> +				end = folio->index;
>  			goto out;
> -
> -		if (folio2->mapping != folio->mapping ||
> -		    !folio_test_large(folio2))
> -			goto out_put;
> -
> -		if (!folio_trylock(folio2))
> -			goto out_put;
> -
> -		if (page_folio(split_at2) != folio2) {
> -			folio_unlock(folio2);
> -			goto out_put;
> -		}
> -		if (!folio_test_large(folio2)) {
> -			end = aligned_end;
> -			folio_unlock(folio2);
> -			goto out_put;
>  		}
> 
> -		/* Split failed: back off to the head of the straddler */
> +		/* Already at the minimum order, nothing to split */
> +		if (folio_order(folio2) == min_order)
> +			goto out_put;
> +
> +		split_at2 = folio_page(folio2, (aligned_end - folio2->index));
> +
> +		/* Split failed, keep the straddler intact */
>  		if (folio_split_or_unmap(folio2, split_at2, min_order))
>  			end = folio2->index;
> -		else
> -			end = aligned_end;
> 
> -		folio_unlock(folio2);
>  out_put:
> +		folio_unlock(folio2);
>  		folio_put(folio2);
>  out:
>  		if (pend)

FWIW this looks much more understandable to me. And since this is a
preexisting issue I agree this should be a separate patch.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR