linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCHES] struct fderr
@ 2024-10-03 23:45 Al Viro
  2024-10-03 23:47 ` Al Viro
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Al Viro @ 2024-10-03 23:45 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Christian Brauner, Jan Kara, Amir Goldstein, Miklos Szeredi

	overlayfs uses struct fd for the things where it does not quite
fit.  There we want not "file reference or nothing" - it's "file reference
or an error".  For pointers we can do that by use of ERR_PTR(); strictly
speaking, that's an abuse of C pointers, but the kernel does make sure
that the uppermost page worth of virtual addresses never gets mapped
and no architecture we care about has those as trap representations.
It might be possible to use the same trick for struct fd; however, for
most of the regular struct fd users that would be a pointless headache -
it would pessimize the code generation for no real benefit.  I considered
a possibility of using represenation of ERR_PTR(-EBADF) for empty struct
fd, but that's far from being consistent among the struct fd users and
that ends up with bad code generation even for the users that want to
treat empty as "fail with -EBADF".

	It's better to introduce a separate type, say, struct fderr.
Representable values:
	* borrowed file reference (address of struct file instance)
	* cloned file reference (address of struct file instance)
	* error (a small negative integer).

	With sane constructors we get rid of the field-by-field mess in
ovl_real_fdget{,_meta}() and have them serve as initializers, always
returning a valid struct fderr value.

	That results in mostly sane code; however, there are several
places where we run into an unpleasant problem - the intended scope
is nested inside inode_lock()/inode_unlock(), with problematic gotos.

	Possible solutions:
1) turn inode_lock() into guard-based thing.  No, with the side of
"fuck, no".  guard() has its uses, but
	* the area covered by it should be _very_ easy to see
	* it should not be mixed with gotos, now *OR* later, so
any subsequent changes in the area should remember not to use those.
	* it should never fall into hands of Markus-level entities.
inode_lock fails all those; guard-based variant _will_ be abstracted
away by well-meaning folks, and it will start spreading.  And existing
users are often long, have non-trivial amounts of goto-based cleanups
in them and lifetimes of the objects involved are not particularly
easy to convert to __cleanup-based variants (to put it very mildly).

We might eventually need to reconsider that, but for now the only sane
policy is "no guard-based variants of VFS locks".

2) turn the scopes into explicit compound statements.

3) take them into helper functions.

The series (in viro/vfs.git #work.fderr) tries both (2) and (3) (the
latter as incremental to the former); I would like to hear opinions
on the relative attractiveness of those variants, first and foremost
from overlayfs folks.  If (3) is preferred, the last two commits of
that branch will be collapsed together; if not - the last commit
simply gets dropped.

Please, review.  Individual patches are in followups.

Changes since the last time it had been posted:
	* s/fd_error/fd_err/, to avoid a conflict
	* s/FD_ERR/FDERR_ERR/, to have constructor names consistent
	* add a followup converting the problematic scopes into
helper calls.

Al Viro (3):
      introduce struct fderr, convert overlayfs uses to that
      experimental: convert fs/overlayfs/file.c to CLASS(...)
      [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock

 fs/overlayfs/file.c  | 211 +++++++++++++++++++++++----------------------------
 include/linux/file.h |  37 +++++++--
 2 files changed, 128 insertions(+), 120 deletions(-)

1/3) introduce struct fderr, convert overlayfs uses to that

Similar to struct fd; unlike struct fd, it can represent
error values.

Accessors:

* fd_empty(f):	true if f represents an error
* fd_file(f):	just as for struct fd it yields a pointer to
		struct file if fd_empty(f) is false.  If
		fd_empty(f) is true, fd_file(f) is guaranteed
		_not_ to be an address of any object (IS_ERR()
		will be true in that case)
* fd_err(f):	if f represents an error, returns that error,
		otherwise the return value is junk.

Constructors:

* ERR_FDERR(-E...):	an instance encoding given error [ERR_FDERR, perhaps?]
* BORROWED_FDERR(file):	if file points to a struct file instance,
			return a struct fderr representing that file
			reference with no flags set.
			if file is an ERR_PTR(-E...), return a struct
			fderr representing that error.
			file MUST NOT be NULL.
* CLONED_FDERR(file):	similar, but in case when file points to
			a struct file instance, set FDPUT_FPUT in flags.

Same destructor as for struct fd; I'm not entirely convinced that
playing with _Generic is a good idea here, but for now let's go
that way...

2/3) experimental: convert fs/overlayfs/file.c to CLASS(...)

There are four places where we end up adding an extra scope
covering just the range from constructor to destructor;
not sure if that's the best way to handle that.

The functions in question are ovl_write_iter(), ovl_splice_write(),
ovl_fadvise() and ovl_copyfile().

I still don't like the way we have to deal with the scopes, but...
use of guard() for inode_lock()/inode_unlock() is a gutter too deep,
as far as I'm concerned.

3/3) [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock

Takes the 4 scopes mentioned in the previous commit into helper
functions.  To be folded into the previous if both go in.

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-10-17 19:47 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-03 23:45 [RFC][PATCHES] struct fderr Al Viro
2024-10-03 23:47 ` Al Viro
2024-10-03 23:47 ` introduce struct fderr, convert overlayfs uses to that Al Viro
2024-10-04  9:43   ` Christian Brauner
2024-10-04 10:47   ` Amir Goldstein
2024-10-17 19:47     ` Al Viro
2024-10-03 23:48 ` [PATCH 2/3] experimental: convert fs/overlayfs/file.c to CLASS(...) Al Viro
2024-10-04  9:40   ` Christian Brauner
2024-10-03 23:48 ` [PATCH 3/3] [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock Al Viro
2024-10-04 10:32 ` [RFC][PATCHES] struct fderr Amir Goldstein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).