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

From: Dave Chinner <david@fromorbit.com>
To: Jan Kara <jack@suse.cz>
Cc: adilger@dilger.ca, tytso@mit.edu, linux-api@vger.kernel.org,
	xfs@oss.sgi.com, Christoph Hellwig <hch@infradead.org>,
	dmonakhov@openvz.org, viro@zeniv.linux.org.uk,
	Li Xi <pkuelelixi@gmail.com>,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 4/4] Adds ioctl interface support for ext4 project
Date: Thu, 25 Sep 2014 17:59:12 +1000	[thread overview]
Message-ID: <20140925075912.GG4758@dastard> (raw)
In-Reply-To: <20140924170105.GE27000@quack.suse.cz>

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