Re: [PATCH 11/11] ext4: add cross rename support

[Miklos Szeredi]

On Sat, Jan 18, 2014 at 5:27 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Sat, Jan 18, 2014 at 07:49:29AM +0100, Miklos Szeredi wrote:
>> On Fri, Jan 17, 2014 at 11:08 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>> > On Fri, Jan 17, 2014 at 11:53:07PM +1300, Michael Kerrisk (man-pages) wrote:
>> >> >        The following additional errors are defined for renameat2():
>> >> >
>> >> >        EOPNOTSUPP
>> >> >               The filesystem does not support a flag in flags
>> >>
>> >> This is not the usual error for an invalid bit flag. Please make it EINVAL.
>> >
>> > I agree that EINVAL makes sense for an invalid bit flag.
>> >
>> > But renameat2() can also fail when the caller passes a perfectly valid
>> > flags field but the paths resolve to a filesystem that doesn't support
>> > the RENAME_EXCHANGE operation.  EOPNOTSUPP looks more appropriate in
>> > that case.
>>
>> OTOH, from the app's perspective, it makes little difference whether a
>> particular kernel doesn't support the reanameat2 syscall, or it
>> doesn't support RENAME_FOO flag or if it does support RENAME_FOO but
>> not in all filesystems.  In all those cases it has to just fall back
>> to something supported and it doesn't matter *why* it wasn't
>> supported.
>
> Well, in theory it could allow an optimization:
>
>         if (kernel_has_foo) {
>                 ret = rename(.,.,.,.,RENAME_FOO);
>                 if (ret && errno == EINVAL)
>                         kernel_has_foo = 0;
>         }
>         if (!kernel_has_foo)
>                 fallback...
>
> or maybe even:
>
>         if (kernel_has_foo && fs_has_foo[fsid])
>                 ret = rename(.,.,.,.,RENAME_FOO);
>                 if (ret && errno == EINVAL)
>                         kernel_has_foo = 0;
>                 if (ret && errno == EOPNOTSUPP)
>                         fs_has_foo[fsid] = 0;
>         }
>         if (!kernel_has_foo || !fs_has_foo[fsid])
>                 fallback...
>
> which may both be of dubious value--unless, say, you're implementing a
> network protocol and making this distinction to your client allows it to
> save server round trips.
>
> That may not be *totally* farfetched--if for example we added rename2 to
> the nfs protocol then servers probably will be required to make this
> sort of distinction per filesystem, exactly to allow client logic like
> the above.

I understand, but that's a protocol issue, not a filesystem issue.
The server will need to determine per-filesystem if the operation is
supported or not, but that doesn't depend on the error value returned
by the filesystem.

> And as long as we can, I'd just rather give the caller more information
> than less.
>
> As for precedent for EOPNOTSUPP: grepping through man-pages the one
> documented use of EOPNOTSUPP I see for filesystems is fallocate, for a
> similar "filesystem doesn't support this operation" case.  "git grep
> EOPNOTSUPP fs/" in the kernel repo suggests there are many more such,
> but I haven't tried to figure out what any of them are.

The reason I chose EOPNOTSUPP is because it has the specific meaning:
"this operation is not supported, try to fall back to something else".
 EINVAL just means "something" is invalid.  That would most likely be
the "flags" argument in this specific case, and hence it works for
renameat2().

And differentiating between the "per-filesystem supported" and the
"per kernel supported" thing based on the error value would also work.
  I don't really have a preference and I don't think it's a big deal.

Michael?

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/