On Wed, Jan 11, 2023 at 07:36:26PM +0000, Matthew Wilcox wrote: > 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 ...) That's awful. Exposing internal implementation details in the API that is supposed to abstract away the internal implementation details from users doesn't seem like a great idea to me. Exactly what are we trying to fix here? Do we really need to punch a hole through the abstraction layers like this just to remove half a dozen lines of -slow path- context specific error handling from a single caller? If there's half a dozen cases that need this sort of handling, then maybe it's the right thing to do. But for a single calling context that only needs to add a null return check in one specific case? There's absolutely no need to make generic infrastructure violate layering abstractions to handle that... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
