On Tue, Jan 10, 2023 at 07:24:27AM -0800, Christoph Hellwig wrote: > On Tue, Jan 10, 2023 at 01:34:16PM +0000, Matthew Wilcox wrote: > > > Exactly. And as I already pointed out in reply to Dave's original > > > patch what we really should be doing is returning an ERR_PTR from > > > __filemap_get_folio instead of reverse-engineering the expected > > > error code. > > > > Ouch, we have a nasty problem. > > > > If somebody passes FGP_ENTRY, we can return a shadow entry. And the > > encodings for shadow entries overlap with the encodings for ERR_PTR, > > meaning that some shadow entries will look like errors. The way I > > solved this in the XArray code is by shifting the error values by > > two bits and encoding errors as XA_ERROR(-ENOMEM) (for example). > > > > I don't _object_ to introducing XA_ERROR() / xa_err() into the VFS, > > but so far we haven't, and I'd like to make that decision intentionally. > > So what would be an alternative way to tell the callers why no folio > was found instead of trying to reverse engineer that? Return an errno > and the folio by reference? The would work, but the calling conventions > would be awful. Agreed. How about an xa_filemap_get_folio()? (there are a number of things to fix here; haven't decided if XA_ERROR should return void *, or whether i should use a separate 'entry' and 'folio' until I know the entry is actually a folio ...) Usage would seem pretty straightforward: folio = xa_filemap_get_folio(iter->inode->i_mapping, pos >> PAGE_SHIFT, fgp, mapping_gfp_mask(iter->inode->i_mapping)); status = xa_err(folio); if (status) goto out_no_page; diff --git a/mm/filemap.c b/mm/filemap.c index 7bf8442bcfaa..7d489f96c690 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1800,40 +1800,25 @@ static void *mapping_get_entry(struct address_space *mapping, pgoff_t index) } /** - * __filemap_get_folio - Find and get a reference to a folio. + * xa_filemap_get_folio - Find and get a reference to a folio. * @mapping: The address_space to search. * @index: The page index. * @fgp_flags: %FGP flags modify how the folio is returned. * @gfp: Memory allocation flags to use if %FGP_CREAT is specified. * - * Looks up the page cache entry at @mapping & @index. - * - * @fgp_flags can be zero or more of these flags: - * - * * %FGP_ACCESSED - The folio will be marked accessed. - * * %FGP_LOCK - The folio is returned locked. - * * %FGP_ENTRY - If there is a shadow / swap / DAX entry, return it - * instead of allocating a new folio to replace it. - * * %FGP_CREAT - If no page is present then a new page is allocated using - * @gfp and added to the page cache and the VM's LRU list. - * The page is returned locked and with an increased refcount. - * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the - * page is already in cache. If the page was allocated, unlock it before - * returning so the caller can do the same dance. - * * %FGP_WRITE - The page will be written to by the caller. - * * %FGP_NOFS - __GFP_FS will get cleared in gfp. - * * %FGP_NOWAIT - Don't get blocked by page lock. - * * %FGP_STABLE - Wait for the folio to be stable (finished writeback) - * - * If %FGP_LOCK or %FGP_CREAT are specified then the function may sleep even - * if the %GFP flags specified for %FGP_CREAT are atomic. + * Looks up the page cache entry at @mapping & @index. See + * __filemap_get_folio() for a detailed description. * - * If there is a page cache page, it is returned with an increased refcount. + * This differs from __filemap_get_folio() in that it will return an + * XArray error instead of NULL if something goes wrong, allowing the + * advanced user to distinguish why the failure happened. We can't use an + * ERR_PTR() because its encodings overlap with shadow/swap/dax entries. * - * Return: The found folio or %NULL otherwise. + * Return: The entry in the page cache or an xa_err() if there is no entry + * or it could not be appropiately locked. */ -struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, - int fgp_flags, gfp_t gfp) +struct folio *xa_filemap_get_folio(struct address_space *mapping, + pgoff_t index, int fgp_flags, gfp_t gfp) { struct folio *folio; @@ -1851,7 +1836,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, if (fgp_flags & FGP_NOWAIT) { if (!folio_trylock(folio)) { folio_put(folio); - return NULL; + return (struct folio *)XA_ERROR(-EAGAIN); } } else { folio_lock(folio); @@ -1890,7 +1875,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, folio = filemap_alloc_folio(gfp, 0); if (!folio) - return NULL; + return (struct folio *)XA_ERROR(-ENOMEM); if (WARN_ON_ONCE(!(fgp_flags & (FGP_LOCK | FGP_FOR_MMAP)))) fgp_flags |= FGP_LOCK; @@ -1902,19 +1887,65 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, err = filemap_add_folio(mapping, folio, index, gfp); if (unlikely(err)) { folio_put(folio); - folio = NULL; if (err == -EEXIST) goto repeat; + folio = (struct folio *)XA_ERROR(err); + } else { + /* + * filemap_add_folio locks the page, and for mmap + * we expect an unlocked page. + */ + if (fgp_flags & FGP_FOR_MMAP) + folio_unlock(folio); } - - /* - * filemap_add_folio locks the page, and for mmap - * we expect an unlocked page. - */ - if (folio && (fgp_flags & FGP_FOR_MMAP)) - folio_unlock(folio); } + if (!folio) + folio = (struct folio *)XA_ERROR(-ENODATA); + return folio; +} +EXPORT_SYMBOL_GPL(xa_filemap_get_folio); + +/** + * __filemap_get_folio - Find and get a reference to a folio. + * @mapping: The address_space to search. + * @index: The page index. + * @fgp: %FGP flags modify how the folio is returned. + * @gfp: Memory allocation flags to use if %FGP_CREAT is specified. + * + * Looks up the page cache entry at @mapping & @index. + * + * @fgp_flags can be zero or more of these flags: + * + * * %FGP_ACCESSED - The folio will be marked accessed. + * * %FGP_LOCK - The folio is returned locked. + * * %FGP_ENTRY - If there is a shadow / swap / DAX entry, return it + * instead of allocating a new folio to replace it. + * * %FGP_CREAT - If no page is present then a new page is allocated using + * @gfp and added to the page cache and the VM's LRU list. + * The page is returned locked and with an increased refcount. + * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the + * page is already in cache. If the page was allocated, unlock it before + * returning so the caller can do the same dance. + * * %FGP_WRITE - The page will be written to by the caller. + * * %FGP_NOFS - __GFP_FS will get cleared in gfp. + * * %FGP_NOWAIT - Don't get blocked by page lock. + * * %FGP_STABLE - Wait for the folio to be stable (finished writeback) + * + * If %FGP_LOCK or %FGP_CREAT are specified then the function may sleep even + * if the %GFP flags specified for %FGP_CREAT are atomic. + * + * If there is a page cache page, it is returned with an increased refcount. + * + * Return: The found folio or %NULL otherwise. + */ +struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, + int fgp, gfp_t gfp) +{ + struct folio *folio = xa_filemap_get_folio(mapping, index, fgp, gfp); + + if (xa_is_err(folio)) + return NULL; return folio; } EXPORT_SYMBOL(__filemap_get_folio);