Re: [PATCH] generic/624: add fstests for idmapped mounts

Re: [PATCH] generic/624: add fstests for idmapped mounts

[Christoph Hellwig]

> +# Modify as appropriate.
> +_supported_fs ext4 xfs

I think this should be

_supported_fs generic

now that we do the probing for the actual support, and any file system
not supporting it will just do a _notrun from _require_idmapped_mounts.

Also the comment above this line should be dropped.

Re: [PATCH] generic/624: add fstests for idmapped mounts

[Darrick J. Wong]

On Mon, Mar 01, 2021 at 06:32:51PM +0100, Christoph Hellwig wrote:
> > +# Modify as appropriate.
> > +_supported_fs ext4 xfs
> 
> I think this should be
> 
> _supported_fs generic
> 
> now that we do the probing for the actual support, and any file system
> not supporting it will just do a _notrun from _require_idmapped_mounts.
> 
> Also the comment above this line should be dropped.

The rest of this series still hasn't shown up in my inbox...

...but I did wonder, have you all tested file creation with idmapped
mounts and quotas enabled?  I noticed after the fact (sorry...) that
the dqalloc call just prior to creating the inode still uses
current_fs[ug]id() and not the fs[ug]id_into_mnt() call that the
conversion series uses.

(The fact that xfs access the ids once for quota and again when actually
creating the inode is screaming for a cleanup, by the way...)

--D

Re: [PATCH] generic/624: add fstests for idmapped mounts

[Christian Brauner]

On Mon, Mar 01, 2021 at 05:43:33PM -0800, Darrick J. Wong wrote:
> On Mon, Mar 01, 2021 at 06:32:51PM +0100, Christoph Hellwig wrote:
> > > +# Modify as appropriate.
> > > +_supported_fs ext4 xfs
> > 
> > I think this should be
> > 
> > _supported_fs generic
> > 
> > now that we do the probing for the actual support, and any file system
> > not supporting it will just do a _notrun from _require_idmapped_mounts.
> > 
> > Also the comment above this line should be dropped.
> 
> The rest of this series still hasn't shown up in my inbox...

This is probably vger being clogged again due to some gmail crap. I'll
paste the patch below in case it hasn't made it onto the list until you
show up but for fwiw the latest version can always found at:

[1]: https://github.com/brauner/xfstests/tree/idmapped_mounts
[2]: https://gitlab.com/brauner/xfstests/-/tree/idmapped_mounts
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/xfstests-dev.git/log/?h=idmapped_mounts

with the main test suite being located under

src/idmapped_mounts/idmapped_mounts.c

> 
> ...but I did wonder, have you all tested file creation with idmapped
> mounts and quotas enabled?  I noticed after the fact (sorry...) that
> the dqalloc call just prior to creating the inode still uses
> current_fs[ug]id() and not the fs[ug]id_into_mnt() call that the
> conversion series uses.

I think I commented on that on the original patch series where that
question was brought up and we concluded that not using the mapping was
the correct thing to do in this codepath.
I'll try to add tests for quotas now and get them back to you by my EOD
hopefully as part of the patch series.

> 
> (The fact that xfs access the ids once for quota and again when actually
> creating the inode is screaming for a cleanup, by the way...)

Yeah. :)

Re: [PATCH] generic/624: add fstests for idmapped mounts

[Christian Brauner]

On Mon, Mar 01, 2021 at 06:32:51PM +0100, Christoph Hellwig wrote:
> > +# Modify as appropriate.
> > +_supported_fs ext4 xfs
> 
> I think this should be
> 
> _supported_fs generic
> 
> now that we do the probing for the actual support, and any file system
> not supporting it will just do a _notrun from _require_idmapped_mounts.
> 
> Also the comment above this line should be dropped.

Will do.

Christian