Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Jan Kara]

On Wed 24-09-14 22:04:30, Li Xi wrote:
> This patch adds ioctl interface for setting/getting project of ext4.
  The patch looks good to me. I was just wondering whether it won't be
useful to add an ioctl() which isn't ext4 specific. We could just extend
->setattr() to allow setting of project ID (most filesystems would just
return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
->setattr from the generic ioctl. That way userspace won't have to care
about filesystem type when setting project ID... What do others think?

								Honza

> Signed-off-by: Li Xi <lixi@ddn.com>
> ---
>  Documentation/filesystems/ext4.txt |    4 ++
>  fs/ext4/ext4.h                     |    2 +
>  fs/ext4/ioctl.c                    |   85 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
> index 919a329..9c98e62 100644
> --- a/Documentation/filesystems/ext4.txt
> +++ b/Documentation/filesystems/ext4.txt
> @@ -609,6 +609,10 @@ EXT4_IOC_SWAP_BOOT	      Swap i_blocks and associated attributes
>  			      The data blocks of the previous boot loader
>  			      will be associated with the given inode.
>  
> + EXT4_IOC_GETPROJECT	      Get project ID associated with inode.
> +
> + EXT4_IOC_SETPROJECT	      Set Project ID associated with inode.
> +
>  ..............................................................................
>  
>  References
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index f8be9bf..51946fd 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -617,6 +617,8 @@ enum {
>  #define EXT4_IOC_RESIZE_FS		_IOW('f', 16, __u64)
>  #define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
>  #define EXT4_IOC_PRECACHE_EXTENTS	_IO('f', 18)
> +#define EXT4_IOC_GETPROJECT		_IOR('f', 19, long)
> +#define EXT4_IOC_SETPROJECT		_IOW('f', 20, long)
>  
>  #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
>  /*
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0f2252e..93b7ff4 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -14,6 +14,8 @@
>  #include <linux/compat.h>
>  #include <linux/mount.h>
>  #include <linux/file.h>
> +#include <linux/quotaops.h>
> +#include <linux/quota.h>
>  #include <asm/uaccess.h>
>  #include "ext4_jbd2.h"
>  #include "ext4.h"
> @@ -611,6 +613,89 @@ resizefs_out:
>  	case EXT4_IOC_PRECACHE_EXTENTS:
>  		return ext4_ext_precache(inode);
>  
> +	case EXT4_IOC_GETPROJECT:
> +	{
> +		__u32 projid;
> +
> +		projid = (__u32)from_kprojid(&init_user_ns,
> +					     EXT4_I(inode)->i_projid);
> +		return put_user(projid, (__u32 __user *) arg);
> +	}
> +	case EXT4_IOC_SETPROJECT:
> +	{
> +		__u32 projid;
> +		int err;
> +		handle_t *handle;
> +		kprojid_t kprojid;
> +		struct ext4_iloc iloc;
> +		struct ext4_inode *raw_inode;
> +
> +		struct dquot *transfer_to[EXT4_MAXQUOTAS] = { };
> +
> +		/* Make sure caller can change project. */
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EACCES;
> +
> +		if (get_user(projid, (__u32 __user *) arg))
> +			return -EFAULT;
> +
> +		kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
> +
> +		if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
> +			return 0;
> +
> +		err = mnt_want_write_file(filp);
> +		if (err)
> +			return err;
> +
> +		err = -EPERM;
> +		mutex_lock(&inode->i_mutex);
> +		/* Is it quota file? Do not allow user to mess with it */
> +		if (IS_NOQUOTA(inode))
> +			goto project_out;
> +
> +		dquot_initialize(inode);
> +
> +		handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
> +			EXT4_QUOTA_INIT_BLOCKS(sb) +
> +			EXT4_QUOTA_DEL_BLOCKS(sb) + 3);
> +		if (IS_ERR(handle)) {
> +			err = PTR_ERR(handle);
> +			goto project_out;
> +		}
> +
> +		err = ext4_reserve_inode_write(handle, inode, &iloc);
> +		if (err)
> +			goto project_stop;
> +
> +		raw_inode = ext4_raw_inode(&iloc);
> +		if ((EXT4_INODE_SIZE(inode->i_sb) <=
> +		     EXT4_GOOD_OLD_INODE_SIZE) ||
> +		    (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))) {
> +		    	err = -EFBIG;
> +		    	goto project_stop;
> +		}
> +
> +		transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
> +		if (!transfer_to[PRJQUOTA])
> +			goto project_set;
> +
> +		err = __dquot_transfer(inode, transfer_to);
> +		dqput(transfer_to[PRJQUOTA]);
> +		if (err)
> +			goto project_stop;
> +
> +project_set:
> +		EXT4_I(inode)->i_projid = kprojid;
> +		inode->i_ctime = ext4_current_time(inode);
> +		err = ext4_mark_iloc_dirty(handle, inode, &iloc);
> +project_stop:
> +		ext4_journal_stop(handle);
> +project_out:
> +		mutex_unlock(&inode->i_mutex);
> +		mnt_drop_write_file(filp);
> +		return err;
> +	}
>  	default:
>  		return -ENOTTY;
>  	}
> -- 
> 1.7.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Christoph Hellwig]

On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> On Wed 24-09-14 22:04:30, Li Xi wrote:
> > This patch adds ioctl interface for setting/getting project of ext4.
>   The patch looks good to me. I was just wondering whether it won't be
> useful to add an ioctl() which isn't ext4 specific. We could just extend
> ->setattr() to allow setting of project ID (most filesystems would just
> return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> ->setattr from the generic ioctl. That way userspace won't have to care
> about filesystem type when setting project ID... What do others think?

Absolutely.  In general I also wonder why this patch doesn't implement
the full XFS API.  Maybe there is a reason it was considered and
rejected, but it would be helpful to document why.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Jan Kara]

On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
> On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> > On Wed 24-09-14 22:04:30, Li Xi wrote:
> > > This patch adds ioctl interface for setting/getting project of ext4.
> >   The patch looks good to me. I was just wondering whether it won't be
> > useful to add an ioctl() which isn't ext4 specific. We could just extend
> > ->setattr() to allow setting of project ID (most filesystems would just
> > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> > ->setattr from the generic ioctl. That way userspace won't have to care
> > about filesystem type when setting project ID... What do others think?
> 
> Absolutely.  In general I also wonder why this patch doesn't implement
> the full XFS API.  Maybe there is a reason it was considered and
> rejected, but it would be helpful to document why.
  Do you mean full get/setfsxattr API? That basically contains project ID,
flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and
extent size hint right? That seems workable and it would also make setting
of PROJINHERIT flag fs agnostic. Only we would have to create some generic
flags namespace and merge into that ext4 flags and have a translation
function for the old ext4 flags. Also I'm afraid we may quickly run out of
32 available flags in xflags so we'd need to extend that. But all this
seems to be doable.
								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Dave Chinner]

On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote:
> On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
> > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> > > On Wed 24-09-14 22:04:30, Li Xi wrote:
> > > > This patch adds ioctl interface for setting/getting project of ext4.
> > >   The patch looks good to me. I was just wondering whether it won't be
> > > useful to add an ioctl() which isn't ext4 specific. We could just extend
> > > ->setattr() to allow setting of project ID (most filesystems would just
> > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> > > ->setattr from the generic ioctl. That way userspace won't have to care
> > > about filesystem type when setting project ID... What do others think?
> > 
> > Absolutely.  In general I also wonder why this patch doesn't implement
> > the full XFS API.  Maybe there is a reason it was considered and
> > rejected, but it would be helpful to document why.
>   Do you mean full get/setfsxattr API?

That's a good start.

The bigger issue in my mind is that we already have a fully featured
quota API that supports project quotas and userspace tools available
that manipulate it. xfstests already uses those tools and API
for testing project quotas.

This whole patchset reinvents all the quota APIs, and will require
adding support in userspace, and hence require re-inventing all the
test infrastructure we already have because it won't be compatible
with the existing project quota test code.

>   That basically contains project ID,
> flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and
> extent size hint right?

It's a different set of flag definitions. We translate the interface
XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just
like we translate the VFS FS_*_FL flags that get passed through the
FS_IOC_GETFLAGS/SETFLAGS ioctl.

> That seems workable and it would also make setting
> of PROJINHERIT flag fs agnostic. Only we would have to create some generic
> flags namespace and merge into that ext4 flags and have a translation
> function for the old ext4 flags.

The XFS_XFLAGS_* are already filesystem agnostic - they are flags
that are only used for interfacing with userspace and hence only
exist at the ioctl copy in/out layer.....

> Also I'm afraid we may quickly run out of
> 32 available flags in xflags so we'd need to extend that. But all this
> seems to be doable.

The struct fsxattr was designed to be extensible - it has unused
padding and enough space in the flags field to allow us to
conditionally use that padding....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[lixi]

在 2014年9月25日，下午3:59，Dave Chinner <david@fromorbit.com> 写道：

> On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote:
>> On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
>>> On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
>>>> On Wed 24-09-14 22:04:30, Li Xi wrote:
>>>>> This patch adds ioctl interface for setting/getting project of ext4.
>>>>  The patch looks good to me. I was just wondering whether it won't be
>>>> useful to add an ioctl() which isn't ext4 specific. We could just extend
>>>> ->setattr() to allow setting of project ID (most filesystems would just
>>>> return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
>>>> ->setattr from the generic ioctl. That way userspace won't have to care
>>>> about filesystem type when setting project ID... What do others think?
>>> 
>>> Absolutely.  In general I also wonder why this patch doesn't implement
>>> the full XFS API.  Maybe there is a reason it was considered and
>>> rejected, but it would be helpful to document why.
>>  Do you mean full get/setfsxattr API?
> 
> That's a good start.
> 
> The bigger issue in my mind is that we already have a fully featured
> quota API that supports project quotas and userspace tools available
> that manipulate it. xfstests already uses those tools and API
> for testing project quotas.
> 
> This whole patchset reinvents all the quota APIs, and will require
> adding support in userspace, and hence require re-inventing all the
> test infrastructure we already have because it won't be compatible
> with the existing project quota test code.
> 
>>  That basically contains project ID,
>> flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and
>> extent size hint right?
> 
> It's a different set of flag definitions. We translate the interface
> XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just
> like we translate the VFS FS_*_FL flags that get passed through the
> FS_IOC_GETFLAGS/SETFLAGS ioctl.
> 
>> That seems workable and it would also make setting
>> of PROJINHERIT flag fs agnostic. Only we would have to create some generic
>> flags namespace and merge into that ext4 flags and have a translation
>> function for the old ext4 flags.
> 
> The XFS_XFLAGS_* are already filesystem agnostic - they are flags
> that are only used for interfacing with userspace and hence only
> exist at the ioctl copy in/out layer.....
> 
>> Also I'm afraid we may quickly run out of
>> 32 available flags in xflags so we'd need to extend that. But all this
>> seems to be doable.
> 
> The struct fsxattr was designed to be extensible - it has unused
> padding and enough space in the flags field to allow us to
> conditionally use that padding….

Hi Dave,

I was mostly working on the semantics of inherit flag on these patches. And
I didn’t realized that the interface differences would bother you so much. Sorry
for that.

I agree that we should choose a good interface. It would be good that it is
general so that it works well for all file systems. I agree that adding an
ext4 specific ioctl() is far from the best choice. I am willing to change it to
any general interface. A general ioctl() sounds good to me. Extend setattr()
and getattr() for project ID sounds even better, since project ID looks like
UID/GID. And general xattr name is another choice. But it is might be a little
bit confusing if we use xattr actually, since we are not saving project ID as
extended attribute on Ext4. Any choice is fine with me, as long as the
implementation won't introduce nasty codes or inconsistent design.

However, the problem is, I do not quite understand why we should keep
the interface exactly the same with XFS. It would be good if we can. But
as far as I can see, it seems hard. XFS uses a lot interfaces which are
not so standard and used by other file systems. For example, struct
fsxattr is not used by other file systems at all except XFS. I am not sure
why we should introduce this into Ext4 if there are a lot of other better
ways. I would be happy to change to XFS interfaces, if it is general.
However, I don’t think it is general enough.

I know xfstest is using the existing project quota interfaces of XFS. And
maybe there are some applications which are using them too. But
keeping the interfaces exactly the same with XFS would cost so much
effort that I’d like to get enough reasons before start working on it. Is it
really necessary? I am not so sure. It is so easy to change user
space applications comparing to changing a weird interfaces. For
example, I think it won’t cost even more than a day to add xfstest
support for new Ext4 project quota. And since project quota is far from
a widely used feature, I don’t think there is much compatibility problems
for existing applications.  And If the new project interface are general
enough, there won’t be any compatibility problems for new applications
at all.

I might missed something important. So please let me know if you have
any advices.

Regards,
                         - Li Xi
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Dave Chinner]

On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote:
> Hi Dave,
> 
> I was mostly working on the semantics of inherit flag on these patches. And
> I didn’t realized that the interface differences would bother you so much. Sorry
> for that.

It's not the differences that bother me - it's the fact that I was
repeatedly ignored until someone else raised the same issue....

> I agree that we should choose a good interface. It would be good that it is
> general so that it works well for all file systems. I agree that adding an
> ext4 specific ioctl() is far from the best choice. I am willing to change it to
> any general interface. A general ioctl() sounds good to me. Extend setattr()
> and getattr() for project ID sounds even better, since project ID looks like
> UID/GID.

Ah, no, project ID is not a uid/gid. It's a completely independent
construct.

[ Which brings me to, once again, the issue of being ignored during
reviews: project IDs should not be mapped by user namespaces, nor be
accessible from anything other than the init namespace.

In XFS we've turned off access to project IDs within namespace
containers because they are used for container space management
(i.e. by the init namespace) to manage the amount of filesystem
space a container or set of containers can use. We do not want
project IDs to be manipulated from within such containers, and
therefore have to prevent access to them from within user
namespaces. ]

> And general xattr name is another choice. But it is might be a little
> bit confusing if we use xattr actually, since we are not saving project ID as
> extended attribute on Ext4. Any choice is fine with me, as long as the
> implementation won't introduce nasty codes or inconsistent design.

We can easily create another ioctl name if we have to. It just needs
sto be defined to the same value as the XFS ioctl names currently
are. We've done this before when making ioctls that originated in
XFS generic (e.g. with freeze/thaw ioctls)....

> However, the problem is, I do not quite understand why we should keep
> the interface exactly the same with XFS. It would be good if we can. But
> as far as I can see, it seems hard. XFS uses a lot interfaces which are
> not so standard and used by other file systems. For example, struct
> fsxattr is not used by other file systems at all except XFS.

Moving a existing structure definitions to a different header file
is too hard?

> I am not sure why we should introduce this into Ext4 if there are
> a lot of other better ways. I would be happy to change to XFS
> interfaces, if it is general.  However, I don’t think it is
> general enough.

How is it not general enough? Examples, please, not handwaving:
which bit of the quota interface can't ext4 use because it's XFS
specific?

We already have a perfectly functional interface and a large body of
code that implements and tests it. You're saying "oh, it's too much
work for me to implement an existing interface" and ignoring the
fact that not implementing the existing interface forces a huge
amount of downstream work. e.g.

	- we need completely new test infrastructure to replicate
	  existing tests.
	- we need new tests to ensure the different APIs and
	  utilities provide the same functionality, and that the
	  work identically.
	- administrators are going to have to learn how ext4 is
	  different to what they already know and understand.
	- administrators that has tools written to manage project
	  quotas is going to have to rewrite them to support ext4.

It's an entirely selfish argument that ignores what already existing
out in userspace. i.e. you're saying that existing downstream users of
project quotas simple don't matter to you...

> I know xfstest is using the existing project quota interfaces of XFS. And
> maybe there are some applications which are using them too. But
> keeping the interfaces exactly the same with XFS would cost so much
> effort that I’d like to get enough reasons before start working on it. Is it
> really necessary? I am not so sure.

You have to have a stronger argument than that to justify creating a
new incompatible user interface. The XFS interfaces have been
available for more than 10 years and support all the functionality
ext4 requires. If it was any other userspace interface (e.g.
syscalls) or any filesystem other than ext4 there would be people
from all over telling you "use the existing interfaces!" and you'd
need very strong reasons for creating a new one.

i.e. you need to demonstrate that the existing interfaces are
inadequate for the intended purpose of the new functionality. That's
clearly not the case here so why should we allow you to create an
incompatible userspace API rather than use the existing, fully
functional API?

> It is so easy to change user space applications comparing to
> changing a weird interfaces.

The existing generic quota tools (i.e quotactl, repquota, etc)
already implement the XFS quota API to be able to query XFS
filesystems.  There's no "changing to wierd interfaces" necessary
for userspace; it's already all there. Hence any work you do to add
project quota awareness to those generic userspace tools will need to
add the support to the XFS queries anyway.

IOWs, you're not making it any easier for yourself in userspace by
creating a new API for ext4 - it just doubles the amount of work you
have to in userspace to make existing tools project quota aware.

> For
> example, I think it won’t cost even more than a day to add xfstest
> support for new Ext4 project quota.

A day of whose time? 

Ever thought about how much time it will take reviewers to look at
your tests and iterate over them to get it all right? If you're
introducing new userspace infrastructure that xfstests will need to
depend on and test for, then it's a lot more than just writing new
tests.

Indeed, I'm likely to want new project quota tests to be generic
(i.e. works and passes on any filesystem that supports project
quotas) with the introduction of ext4 project quota support. It's
the same functionality and so it should work the same just like user
and group quotas do across all filesystems.

> And since project quota is far from
> a widely used feature,

I don't think you realise quite how widespread it's use is on XFS.

> I don’t think there is much compatibility problems
> for existing applications.  And If the new project interface are general
> enough, there won’t be any compatibility problems for new applications
> at all.

Again, you are ignoring the compatibility problems with existing
applications that are project quota aware. For them you are
*creating new compatibility problems* by implementing a new
interface. i.e. Existing applications will not work on ext4, and
new applications written to work on ext4 won't work on XFS.

That's the crux of the issue - we have existing applications using
the existing interface and so introducing a new interface introduces
compatibility problems.  You can't just wave this problem away
because you don't think the existing interface matters.

"It's easier for me to create a new interface" is not a valid reason
for creating a new interface....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Li Xi]

On Fri, Sep 26, 2014 at 8:10 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Sep 25, 2014 at 07:34:38PM +0800, lixi wrote:
>> Hi Dave,
>>
>> I was mostly working on the semantics of inherit flag on these patches. And
>> I didn’t realized that the interface differences would bother you so much. Sorry
>> for that.
>
> It's not the differences that bother me - it's the fact that I was
> repeatedly ignored until someone else raised the same issue....
Sorry about that. Since we are implementing a feature which exsits in XFS
for years, your opions are surely very important. They are not ignored.
But I think more discussion is necessary before an common agreement is
reached. And I am willing to change the interfaces to whatever all people
agree on.
>
>> I agree that we should choose a good interface. It would be good that it is
>> general so that it works well for all file systems. I agree that adding an
>> ext4 specific ioctl() is far from the best choice. I am willing to change it to
>> any general interface. A general ioctl() sounds good to me. Extend setattr()
>> and getattr() for project ID sounds even better, since project ID looks like
>> UID/GID.
>
> Ah, no, project ID is not a uid/gid. It's a completely independent
> construct.
>
> [ Which brings me to, once again, the issue of being ignored during
> reviews: project IDs should not be mapped by user namespaces, nor be
> accessible from anything other than the init namespace.
>
> In XFS we've turned off access to project IDs within namespace
> containers because they are used for container space management
> (i.e. by the init namespace) to manage the amount of filesystem
> space a container or set of containers can use. We do not want
> project IDs to be manipulated from within such containers, and
> therefore have to prevent access to them from within user
> namespaces. ]
OK. That is another semantics which needs further discussion whether
we need to implement for Ext4. Container space management is one
of the project quota use case. I am not sure how widely used it is,
but I like features with less limits.
>
>> And general xattr name is another choice. But it is might be a little
>> bit confusing if we use xattr actually, since we are not saving project ID as
>> extended attribute on Ext4. Any choice is fine with me, as long as the
>> implementation won't introduce nasty codes or inconsistent design.
>
> We can easily create another ioctl name if we have to. It just needs
> sto be defined to the same value as the XFS ioctl names currently
> are. We've done this before when making ioctls that originated in
> XFS generic (e.g. with freeze/thaw ioctls)....
OK. I am fine with general ioctl to set/get project ID.
>
>> However, the problem is, I do not quite understand why we should keep
>> the interface exactly the same with XFS. It would be good if we can. But
>> as far as I can see, it seems hard. XFS uses a lot interfaces which are
>> not so standard and used by other file systems. For example, struct
>> fsxattr is not used by other file systems at all except XFS.
>
> Moving a existing structure definitions to a different header file
> is too hard?
I don't think difficulty is a big problem. I'm wondering whether it is
necessary.
>
>> I am not sure why we should introduce this into Ext4 if there are
>> a lot of other better ways. I would be happy to change to XFS
>> interfaces, if it is general.  However, I don’t think it is
>> general enough.
>
> How is it not general enough? Examples, please, not handwaving:
> which bit of the quota interface can't ext4 use because it's XFS
> specific?
Well, I am not so familar with XFS, so I might be all wrong about
XFS. But honestly, a lot of quota interfaces of XFS seems not
so standard. For example, all the Q_X* flags used in do_quotactl().
Maybe there are really good reasons why they are there. I don't
know. And XFS does not use general codes under fs/quota. That
is a big difference with Ext4.
>
> We already have a perfectly functional interface and a large body of
> code that implements and tests it. You're saying "oh, it's too much
> work for me to implement an existing interface" and ignoring the
> fact that not implementing the existing interface forces a huge
> amount of downstream work. e.g.
>
>         - we need completely new test infrastructure to replicate
>           existing tests.
>         - we need new tests to ensure the different APIs and
>           utilities provide the same functionality, and that the
>           work identically.
>         - administrators are going to have to learn how ext4 is
>           different to what they already know and understand.
>         - administrators that has tools written to manage project
>           quotas is going to have to rewrite them to support ext4.
>
> It's an entirely selfish argument that ignores what already existing
> out in userspace. i.e. you're saying that existing downstream users of
> project quotas simple don't matter to you...
>
>> I know xfstest is using the existing project quota interfaces of XFS. And
>> maybe there are some applications which are using them too. But
>> keeping the interfaces exactly the same with XFS would cost so much
>> effort that I’d like to get enough reasons before start working on it. Is it
>> really necessary? I am not so sure.
>
> You have to have a stronger argument than that to justify creating a
> new incompatible user interface. The XFS interfaces have been
> available for more than 10 years and support all the functionality
> ext4 requires. If it was any other userspace interface (e.g.
> syscalls) or any filesystem other than ext4 there would be people
> from all over telling you "use the existing interfaces!" and you'd
> need very strong reasons for creating a new one.
>
> i.e. you need to demonstrate that the existing interfaces are
> inadequate for the intended purpose of the new functionality. That's
> clearly not the case here so why should we allow you to create an
> incompatible userspace API rather than use the existing, fully
> functional API?
>
>> It is so easy to change user space applications comparing to
>> changing a weird interfaces.
>
> The existing generic quota tools (i.e quotactl, repquota, etc)
> already implement the XFS quota API to be able to query XFS
> filesystems.  There's no "changing to wierd interfaces" necessary
> for userspace; it's already all there. Hence any work you do to add
> project quota awareness to those generic userspace tools will need to
> add the support to the XFS queries anyway.
>
> IOWs, you're not making it any easier for yourself in userspace by
> creating a new API for ext4 - it just doubles the amount of work you
> have to in userspace to make existing tools project quota aware.
I am afraid I have to make existing tools project quota aware. And
actually I've done most of the work. I've updated e2fsprogs and
quota-tools to support project quota. Unfortunately these updates are
inevitable anyway. As Jan Kara said, we can't force system admins
to change from quota-tool command to xfs quota tools. Thus, I add
'-P $PROJECT' arguments to all the commands. And based on
those tools, I made a script which is less than 1K lines for
regression test. It is working pretty well. I don't see a good reason
why it is necessary to change everything to XFS way.
>
>> For
>> example, I think it won’t cost even more than a day to add xfstest
>> support for new Ext4 project quota.
>
> A day of whose time?
I am always willing to help if you agree. :)
>
> Ever thought about how much time it will take reviewers to look at
> your tests and iterate over them to get it all right? If you're
> introducing new userspace infrastructure that xfstests will need to
> depend on and test for, then it's a lot more than just writing new
> tests.
>
> Indeed, I'm likely to want new project quota tests to be generic
> (i.e. works and passes on any filesystem that supports project
> quotas) with the introduction of ext4 project quota support. It's
> the same functionality and so it should work the same just like user
> and group quotas do across all filesystems.
>
>> And since project quota is far from
>> a widely used feature,
>
> I don't think you realise quite how widespread it's use is on XFS.
>
>> I don’t think there is much compatibility problems
>> for existing applications.  And If the new project interface are general
>> enough, there won’t be any compatibility problems for new applications
>> at all.
>
> Again, you are ignoring the compatibility problems with existing
> applications that are project quota aware. For them you are
> *creating new compatibility problems* by implementing a new
> interface. i.e. Existing applications will not work on ext4, and
> new applications written to work on ext4 won't work on XFS.
>
> That's the crux of the issue - we have existing applications using
> the existing interface and so introducing a new interface introduces
> compatibility problems.  You can't just wave this problem away
> because you don't think the existing interface matters.
>
> "It's easier for me to create a new interface" is not a valid reason
> for creating a new interface....
Sorry about my ignorance about the existing usage of XFS
project quota. I hope it is widely used. But does it really matters
for XFS that what kind of Ext4 interfaces is going to use?
Existing appplications would run happily on XFS any way
using the exisitng XFS interfaces. And if you are concerning
about the compatibility between Ext4 and XFS, I am afraid
those applications have to be changed any way when been ported
to Ext4. Since those applications are using XFS specific feature,
i.e. project quota, it is likely they are using other kind of XFS
speicific features which probably will never be implemented on
Ext4. I don't think there is any easy way to port them from
XFS to Ext4 any way. And I really don't think therea are many
such kind of applications. So, since we are not implementing
interfaces for XFS2 or XFS3, I don't think compatibility problem
is so critical.

Regards,
                                             - Li Xi

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Theodore Ts'o]

On Thu, Sep 25, 2014 at 05:59:12PM +1000, Dave Chinner wrote:
> > Also I'm afraid we may quickly run out of
> > 32 available flags in xflags so we'd need to extend that. But all this
> > seems to be doable.
> 
> The struct fsxattr was designed to be extensible - it has unused
> padding and enough space in the flags field to allow us to
> conditionally use that padding....

I agree that it would be useful for ext4 to support as much of the
XFS_IOC_GETXATTR/XFS_IOC_SETATTR as would make sense for ext4, and to
use that to set/get the project ID.  (And that we should probably do
that as a separate set of patches that we could potentially go into
ext4 ahead of the project quota while it is undergoing testing and
review.)

A few questions of Dave and other XFS folks:

1) If we only implement a partial set of the flags or other
functionality, are there going to be tools that get confused?  i.e.,
are there any userspace programs that will test for whether the ioctl
is supported, and then assume that some minimal set of functionality
must be implemented?

2) Unless I'm missing something, there is nothing that enforces that
fsx_pad must be zero.  I assume that means that the only way you can
expand use of fields into that space is via a flag bit being consumed?

Cheers,

						- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Dave Chinner]

On Thu, Sep 25, 2014 at 09:41:37AM -0400, Theodore Ts'o wrote:
> On Thu, Sep 25, 2014 at 05:59:12PM +1000, Dave Chinner wrote:
> > > Also I'm afraid we may quickly run out of
> > > 32 available flags in xflags so we'd need to extend that. But all this
> > > seems to be doable.
> > 
> > The struct fsxattr was designed to be extensible - it has unused
> > padding and enough space in the flags field to allow us to
> > conditionally use that padding....
> 
> I agree that it would be useful for ext4 to support as much of the
> XFS_IOC_GETXATTR/XFS_IOC_SETATTR as would make sense for ext4, and to
> use that to set/get the project ID.  (And that we should probably do
> that as a separate set of patches that we could potentially go into
> ext4 ahead of the project quota while it is undergoing testing and
> review.)
> 
> A few questions of Dave and other XFS folks:
> 
> 1) If we only implement a partial set of the flags or other
> functionality, are there going to be tools that get confused?  i.e.,
> are there any userspace programs that will test for whether the ioctl
> is supported, and then assume that some minimal set of functionality
> must be implemented?

No, I don't think they will get confused.

The use of the flags is get/modify/set just like other flag setting
functions. The extsize and projid fields are condition on the
relevant flag being set on return from a get (i.e. projid is only
valid if XFS_XFLAG_PROJID[_INHERIT] is set), and those fields are
only considered valid on set if those flags are set by the
application (or remain set as a result of the getxattr).

Hence the applications that use the getxattr/setxattr interface
correctly shouldn't care what set of flags and values the filesystem
supports other than the specific flags the application needs the
filesystem to understand.

> 2) Unless I'm missing something, there is nothing that enforces that
> fsx_pad must be zero.  I assume that means that the only way you can
> expand use of fields into that space is via a flag bit being consumed?

Yup, that's exactly what I meant by "conditionally use the padding".
Even if the padding was guaranteed to be zero, I'd strongly
recommend a flag bit to indicate the application understands that
the padding region has actual meaning to guard against buggy
applications.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Jan Kara]

On Thu 25-09-14 17:59:12, Dave Chinner wrote:
> On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote:
> > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
> > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> > > > On Wed 24-09-14 22:04:30, Li Xi wrote:
> > > > > This patch adds ioctl interface for setting/getting project of ext4.
> > > >   The patch looks good to me. I was just wondering whether it won't be
> > > > useful to add an ioctl() which isn't ext4 specific. We could just extend
> > > > ->setattr() to allow setting of project ID (most filesystems would just
> > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> > > > ->setattr from the generic ioctl. That way userspace won't have to care
> > > > about filesystem type when setting project ID... What do others think?
> > > 
> > > Absolutely.  In general I also wonder why this patch doesn't implement
> > > the full XFS API.  Maybe there is a reason it was considered and
> > > rejected, but it would be helpful to document why.
> >   Do you mean full get/setfsxattr API?
> 
> That's a good start.
> 
> The bigger issue in my mind is that we already have a fully featured
> quota API that supports project quotas and userspace tools available
> that manipulate it. xfstests already uses those tools and API
> for testing project quotas.
  Well, the VFS quota API is trivially extended by adding additional quota
type so I don't really see about which reinventing of quota API are you
speaking here...

> This whole patchset reinvents all the quota APIs, and will require
> adding support in userspace, and hence require re-inventing all the
> test infrastructure we already have because it won't be compatible
> with the existing project quota test code.
  Well, quota-tools will have to extended to know about the new quota type.
Yes. But that's easy to do. I think teaching xfs quota tools to work with
ext4 will be a bigger project plus I don't think I want to force sysadmins
which are used to work with quota-tools to switch to other utilities just
because of project quotas.

Regarding xfstests - I've checked and most of the project quota tests in
xfs directory aren't directly usable for ext4 anyway because of other
functionality ext4 doesn't support. So we'll need to distill the least
common denominator from them anyway...

> >   That basically contains project ID,
> > flags (those that are currently get/set with FS_IOC_GETFLAGS/SETFLAGS), and
> > extent size hint right?
> 
> It's a different set of flag definitions. We translate the interface
> XFS_XFLAG_* values to/from the inode on-disk XFS_DIFLAG_* inode values, just
> like we translate the VFS FS_*_FL flags that get passed through the
> FS_IOC_GETFLAGS/SETFLAGS ioctl.
> 
> > That seems workable and it would also make setting
> > of PROJINHERIT flag fs agnostic. Only we would have to create some generic
> > flags namespace and merge into that ext4 flags and have a translation
> > function for the old ext4 flags.
> 
> The XFS_XFLAGS_* are already filesystem agnostic - they are flags
> that are only used for interfacing with userspace and hence only
> exist at the ioctl copy in/out layer.....
> 
> > Also I'm afraid we may quickly run out of
> > 32 available flags in xflags so we'd need to extend that. But all this
> > seems to be doable.
> 
> The struct fsxattr was designed to be extensible - it has unused
> padding and enough space in the flags field to allow us to
> conditionally use that padding....
  Yeah, this should all be easy.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Dave Chinner]

On Thu, Sep 25, 2014 at 03:52:13PM +0200, Jan Kara wrote:
> On Thu 25-09-14 17:59:12, Dave Chinner wrote:
> > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote:
> > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
> > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> > > > > On Wed 24-09-14 22:04:30, Li Xi wrote:
> > > > > > This patch adds ioctl interface for setting/getting project of ext4.
> > > > >   The patch looks good to me. I was just wondering whether it won't be
> > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend
> > > > > ->setattr() to allow setting of project ID (most filesystems would just
> > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> > > > > ->setattr from the generic ioctl. That way userspace won't have to care
> > > > > about filesystem type when setting project ID... What do others think?
> > > > 
> > > > Absolutely.  In general I also wonder why this patch doesn't implement
> > > > the full XFS API.  Maybe there is a reason it was considered and
> > > > rejected, but it would be helpful to document why.
> > >   Do you mean full get/setfsxattr API?
> > 
> > That's a good start.
> > 
> > The bigger issue in my mind is that we already have a fully featured
> > quota API that supports project quotas and userspace tools available
> > that manipulate it. xfstests already uses those tools and API
> > for testing project quotas.
>   Well, the VFS quota API is trivially extended by adding additional quota
> type so I don't really see about which reinventing of quota API are you
> speaking here...

It doesn't seem quite as trivial as you make out given  all thei
issues so far around increasing MAXQUOTA, the increase in size of
the inode, etc.

> > This whole patchset reinvents all the quota APIs, and will require
> > adding support in userspace, and hence require re-inventing all the
> > test infrastructure we already have because it won't be compatible
> > with the existing project quota test code.
>   Well, quota-tools will have to extended to know about the new quota type.
> Yes. But that's easy to do. I think teaching xfs quota tools to work with
> ext4 will be a bigger project plus I don't think I want to force sysadmins
> which are used to work with quota-tools to switch to other utilities just
> because of project quotas.
> 
> Regarding xfstests - I've checked and most of the project quota tests in
> xfs directory aren't directly usable for ext4 anyway because of other
> functionality ext4 doesn't support. So we'll need to distill the least
> common denominator from them anyway...

I just did a quick scan - of the ~13 tests in tests/xfs that
exercise project quotas, only 2 of them test things that are xfs
specific (e.g. use xfs_db to peer at things, or use xfs_admin, etc).
The rest all rely on xfs_quota to manage and configure project
quotas but otherwise don't do anything XFS specific.

We want project quotas to have the same management interface for
administrators regardless of the filesystem they are using. The only
way we can do that is to ensure that the same tools work on either
filesystem, and right now it seems to me that the ext4 NIH syndrome
is winning out over what is best for our users...

Look, I have no problems with extending the existing quota
interfaces to support project quotas, but that should be a
*secondary* improvement as userspace tools are updated. The
primary goal needs to be "works identically to XFS" and so it needs
to implement the interfaces that are currently used for management
so that we can actually test that it does work identically.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Theodore Ts'o]

On Fri, Sep 26, 2014 at 08:42:25AM +1000, Dave Chinner wrote:
> Look, I have no problems with extending the existing quota
> interfaces to support project quotas, but that should be a
> *secondary* improvement as userspace tools are updated. The
> primary goal needs to be "works identically to XFS" and so it needs
> to implement the interfaces that are currently used for management
> so that we can actually test that it does work identically.

I think we're getting a bit too hung up on which is the "primary" and
which is the "secondary" interfaces.  The reality is that we should
make both interfaces work.  An example of this is how we handle the
xfs-specific ioctls that are also exposed via the fallocate(2) system
call.

It's not particularly important to me which is the "primary" interface
just because it's been around for 10 years.  Which should implement
*both* so that users are used to using xfs_io(8) or fallocate(1) can
do what they want.

Similarly, if some users are more used to the quotatools interface
(which has been around for quite a long time, BTW, if we're trying to
count primacy by years of availability to Linux users), and some users
that are used to using xfs_quota, both should work.

Cheers,

						- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Jan Kara]

On Fri 26-09-14 08:42:25, Dave Chinner wrote:
> On Thu, Sep 25, 2014 at 03:52:13PM +0200, Jan Kara wrote:
> > On Thu 25-09-14 17:59:12, Dave Chinner wrote:
> > > On Wed, Sep 24, 2014 at 07:01:05PM +0200, Jan Kara wrote:
> > > > On Wed 24-09-14 09:26:34, Christoph Hellwig wrote:
> > > > > On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> > > > > > On Wed 24-09-14 22:04:30, Li Xi wrote:
> > > > > > > This patch adds ioctl interface for setting/getting project of ext4.
> > > > > >   The patch looks good to me. I was just wondering whether it won't be
> > > > > > useful to add an ioctl() which isn't ext4 specific. We could just extend
> > > > > > ->setattr() to allow setting of project ID (most filesystems would just
> > > > > > return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> > > > > > ->setattr from the generic ioctl. That way userspace won't have to care
> > > > > > about filesystem type when setting project ID... What do others think?
> > > > > 
> > > > > Absolutely.  In general I also wonder why this patch doesn't implement
> > > > > the full XFS API.  Maybe there is a reason it was considered and
> > > > > rejected, but it would be helpful to document why.
> > > >   Do you mean full get/setfsxattr API?
> > > 
> > > That's a good start.
> > > 
> > > The bigger issue in my mind is that we already have a fully featured
> > > quota API that supports project quotas and userspace tools available
> > > that manipulate it. xfstests already uses those tools and API
> > > for testing project quotas.
> >   Well, the VFS quota API is trivially extended by adding additional quota
> > type so I don't really see about which reinventing of quota API are you
> > speaking here...
> 
> It doesn't seem quite as trivial as you make out given  all thei
> issues so far around increasing MAXQUOTA, the increase in size of
> the inode, etc.
  Well, troubles with increasing MAXQUOTAS is more about expectations
that VFS quota subsystem can support only 2 quota types. So we have to do
those changes regardless of interface we choose.

There is one change necessary in the interface (not done yet) and that is
that filesystems using VFS quotas and not supporting project quotas will
need to refuse quotactls for project quotas. This won't be necessary if we
simply refuse to manipulate project quotas using the standard VFS
interface. But I wouldn't call it difficult either.

> > > This whole patchset reinvents all the quota APIs, and will require
> > > adding support in userspace, and hence require re-inventing all the
> > > test infrastructure we already have because it won't be compatible
> > > with the existing project quota test code.
> >   Well, quota-tools will have to extended to know about the new quota type.
> > Yes. But that's easy to do. I think teaching xfs quota tools to work with
> > ext4 will be a bigger project plus I don't think I want to force sysadmins
> > which are used to work with quota-tools to switch to other utilities just
> > because of project quotas.
> > 
> > Regarding xfstests - I've checked and most of the project quota tests in
> > xfs directory aren't directly usable for ext4 anyway because of other
> > functionality ext4 doesn't support. So we'll need to distill the least
> > common denominator from them anyway...
> 
> I just did a quick scan - of the ~13 tests in tests/xfs that
> exercise project quotas, only 2 of them test things that are xfs
> specific (e.g. use xfs_db to peer at things, or use xfs_admin, etc).
> The rest all rely on xfs_quota to manage and configure project
> quotas but otherwise don't do anything XFS specific.
  Yeah, I messed up my original check (I originally found like 5 project
quota related tests and they were those problematic). I checked again and
most of them should be relatively easy to adapt (we'll need some changes
for mount options handling but that's inevitable).

> We want project quotas to have the same management interface for
> administrators regardless of the filesystem they are using. The only
> way we can do that is to ensure that the same tools work on either
> filesystem, and right now it seems to me that the ext4 NIH syndrome
> is winning out over what is best for our users...
> 
> Look, I have no problems with extending the existing quota
> interfaces to support project quotas, but that should be a
> *secondary* improvement as userspace tools are updated. The
> primary goal needs to be "works identically to XFS" and so it needs
> to implement the interfaces that are currently used for management
> so that we can actually test that it does work identically.
  So I had another look at the quotactl interface we are speaking about.
XFS has the following commands there:
Q_XQUOTAON
Q_XQUOTAOFF
  These could be relatively easily hooked up to call appropriate VFS
  functions.

Q_XQUOTARM
  This doesn't have equivalent in VFS and currently I'm not convinced we
want to do the work in filesystems to support this...

Q_XGETQSTAT
Q_XGETQSTATV
  This corresponds to Q_GETINFO of VFS quotas although it provides more
information. We don't easily have things like number of incore dquots or
number of file extents available. There's also no limit on the number of
warnings. But other than that diverting these to VFS interfaces through a
translation function should be easy. Any idea on what numbers should we
present from VFS?

BTW how do you set the information? We have Q_SETINFO for that in VFS
quotas.

Q_XSETQLIM
Q_XGETQUOTA
  These are already handled so they work regardless of the underlying fs
type.

Q_XQUOTASYNC
  This is the same as Q_SYNC. For VFS quotas we need to do some work in some
cases.

  So all in all it seems relatively easy to make the VFS and XFS quota
interfaces more compatible than they are now and it's a direction I like.
I can have a look into that once I finish patches to move i_dquot[] array
out of generic inode (which is desirable regardless of project quotas).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] Adds ioctl interface support for ext4 project

[Dave Chinner]

On Wed, Sep 24, 2014 at 06:25:07PM +0200, Jan Kara wrote:
> On Wed 24-09-14 22:04:30, Li Xi wrote:
> > This patch adds ioctl interface for setting/getting project of ext4.
>   The patch looks good to me. I was just wondering whether it won't be
> useful to add an ioctl() which isn't ext4 specific. We could just extend
> ->setattr() to allow setting of project ID (most filesystems would just
> return -EOPNOTSUPP but ext4 and xfs could do the right thing) and then call
> ->setattr from the generic ioctl. That way userspace won't have to care
> about filesystem type when setting project ID... What do others think?

I've repeatedly said that these ext4 project ID patches should
implement the same interfaces as XFS rather than creating a new set
of incompatible, ext4 specific interfaces to do implement the same
functionality.

There is no good reason for forcing userspace to re-invent tools
that already exist just to manage identical functionality in
different filesystems.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs