linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kevin Locke <kevin@kevinlocke.name>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>,
	overlayfs <linux-unionfs@vger.kernel.org>,
	Miklos Szeredi <mszeredi@redhat.com>
Subject: Re: [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs
Date: Mon, 18 Oct 2021 08:02:48 -0600	[thread overview]
Message-ID: <YW1+iBBsX+Wlfx8O@kevinlocke.name> (raw)
In-Reply-To: <CAJfpegsRo3e-9B64W37YrmvDcjo0QB9t+coAW3mO6TSqdROz2w@mail.gmail.com>

On Mon, 2021-10-18 at 10:47 +0200, Miklos Szeredi wrote:
> On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@kevinlocke.name> wrote:
>> With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename
>> a file fails with -EINVAL on an overlayfs mount with a lower
>> filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS).
>>
>> [...]
>>
>> This issue does not occur on 5.14.  I've bisected the regression to
>> 72db82115d2b.
> 
> This is clearly a regression.  Not trivial how far the fix should go, though.
> 
> One option is to just ignore all errors from ovl_copy_fileattr(),
> which would solve this and similar issues.  However that would result
> in missing the cases when the attributes were really meant to be
> copied up, but failed to do so for some reason.
> 
> If vfs_fileattr_get() fails with ENOIOCTLCMD or ENOTTY on lower, that
> obviously means we need to return success (lower fs does not support
> fileattr).   As ntfs-3g seems to return EINVAL that needs to be added
> too.

I agree.  I like your approach.  Especially for ENOIOCTLCMD/ENOTTY.
Since ntfs-3g returns EINVAL, that seems necessary to avoid logspam
for now.  Do you think it would make sense for me to suggest returning
ENOIOCTLCMD to the ntfs-3g project?  It seems more appropriate and
consistent with other filesystems to me, but I'm certainly not an
expert:
https://github.com/tuxera/ntfs-3g/blob/2021.8.22/libntfs-3g/ioctl.c#L415
I didn't see any other errors from in-tree filesystems, but who knows
errors other FUSE projects may return.

> More interesting question is what to do with get/set failures on
> upper.   My feeling is that for now we should try to return errors
> (even ENOTTY), but should print a warning in the kernel log.  If that
> turns out to regress some use cases, then that needs to fixed as well.

Does that mean all copy-up operations would fail for an upper which
does not support file attributes, or only ones where the file being
copied has attributes set?  I'm a little concerned about debugability,
since the failure modes may not be obvious to users.  Although the log
warnings would help with that.

Would it be possible to refuse to mount an upper which doesn't support
file attrs over a lower which does?  At least the failure would be
immediate and obvious.  If a use case is found, perhaps a mount option
to control file attr behavior cold be added at that point?  Just
thinking out loud.

> Untested patch attached.

Works great for me.

Tested-by: Kevin Locke <kevin@kevinlocke.name>

Thanks for the fast investigation and fix!
Kevin

  reply	other threads:[~2021-10-18 14:16 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210910001820.174272-1-sashal@kernel.org>
2021-09-10  0:17 ` [PATCH AUTOSEL 5.13 40/88] ovl: copy up sync/noatime fileattr flags Sasha Levin
2021-10-17 20:46   ` [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs Kevin Locke
2021-10-18  8:47     ` Miklos Szeredi
2021-10-18 14:02       ` Kevin Locke [this message]
2021-09-10  0:17 ` [PATCH AUTOSEL 5.13 41/88] ovl: skip checking lower file's i_writecount on truncate Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YW1+iBBsX+Wlfx8O@kevinlocke.name \
    --to=kevin@kevinlocke.name \
    --cc=amir73il@gmail.com \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=mszeredi@redhat.com \
    --subject='Re: [Regression] ovl: rename(2) EINVAL if lower doesn'\''t support fileattrs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).