quota: add new quotactl Q_XGETQUOTA2

Factor xfs_seek_hole_data into an unlocked helper which takes an xfs inode rather than a file for internal use.

Also allow specification of "end" - the vfs lseek interface is defined such that any offset past eof/i_size shall return -ENXIO, but we will use this for quota code which does not maintain i_size, and we want to be able to SEEK_DATA past i_size as well. So the lseek path can send in i_size, and the quota code can determine its own ending offset.

Signed-off-by Eric Sandeen <[email protected]>

---
fs/xfs/xfs_file.c | 82 ++++++++++++++++++++++++++++++++++++---------------- fs/xfs/xfs_inode.h | 2 +

2 files changed, 59 insertions(+), 25 deletions(-)

fs/xfs/xfs_file.c +57 -25

index ebe9b82..5dc7113 100644
@@ -1337,31 +1337,31 @@ out:
 return found;
 }
 
-STATIC loff_t
-xfs_seek_hole_data(
- struct file *file,
+/*
+ * caller must lock inode with xfs_ilock_data_map_shared,
+ * can we craft an appropriate ASSERT?
+ *
+ * end is because the VFS-level lseek interface is defined such that any
+ * offset past i_size shall return -ENXIO, but we use this for quota code
+ * which does not maintain i_size, and we want to SEEK_DATA past i_size.
+ */
+loff_t
+__xfs_seek_hole_data(
+ struct inode *inode,
 loff_t start,
+ loff_t end,
 int whence)
 {
- struct inode *inode = file->f_mapping->host;
 struct xfs_inode *ip = XFS_I(inode);
 struct xfs_mount *mp = ip->i_mount;
 loff_t uninitialized_var(offset);
- xfs_fsize_t isize;
 xfs_fileoff_t fsbno;
- xfs_filblks_t end;
- uint lock;
+ xfs_filblks_t lastbno;
 int error;
 
- if (XFS_FORCED_SHUTDOWN(mp))
- return -EIO;
-
- lock = xfs_ilock_data_map_shared(ip);
-
- isize = i_size_read(inode);
- if (start >= isize) {
+ if (start >= end) {
 error = -ENXIO;
- goto out_unlock;
+ goto out_error;
 }
 
 /*

@@ -1369,22 +1369,22 @@ xfs_seek_hole_data(
 * by fsbno to the end block of the file.
 */
 fsbno = XFS_B_TO_FSBT(mp, start);
- end = XFS_B_TO_FSB(mp, isize);
+ lastbno = XFS_B_TO_FSB(mp, end);
 
 for (;;) {
 struct xfs_bmbt_irec map[2];
 int nmap = 2;
 unsigned int i;
 
- error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
+ error = xfs_bmapi_read(ip, fsbno, lastbno - fsbno, map, &nmap,
 XFS_BMAPI_ENTIRE);
 if (error)
- goto out_unlock;
+ goto out_error;
 
 /* No extents at given offset, must be beyond EOF */
 if (nmap == 0) {
 error = -ENXIO;
- goto out_unlock;
+ goto out_error;
 }
 
 for (i = 0; i < nmap; i++) {

@@ -1426,7 +1426,7 @@ xfs_seek_hole_data(
 * hole at the end of any file).
 */
 if (whence == SEEK_HOLE) {
- offset = isize;
+ offset = end;
 break;
 }
 /*

@@ -1434,7 +1434,7 @@ xfs_seek_hole_data(
 */
 ASSERT(whence == SEEK_DATA);
 error = -ENXIO;
- goto out_unlock;
+ goto out_error;
 }
 
 ASSERT(i > 1);

@@ -1445,14 +1445,14 @@ xfs_seek_hole_data(
 */
 fsbno = map[i - 1].br_startoff + map[i - 1].br_blockcount;
 start = XFS_FSB_TO_B(mp, fsbno);
- if (start >= isize) {
+ if (start >= end) {
 if (whence == SEEK_HOLE) {
- offset = isize;
+ offset = end;
 break;
 }
 ASSERT(whence == SEEK_DATA);
 error = -ENXIO;
- goto out_unlock;
+ goto out_error;
 }
 }
 

@@ -1464,7 +1464,39 @@ out:
 * situation in particular.
 */
 if (whence == SEEK_HOLE)
- offset = min_t(loff_t, offset, isize);
+ offset = min_t(loff_t, offset, end);
+
+ return offset;
+
+out_error:
+ return error;
+}
+
+STATIC loff_t
+xfs_seek_hole_data(
+ struct file *file,
+ loff_t start,
+ int whence)
+{
+ struct inode *inode = file->f_mapping->host;
+ struct xfs_inode *ip = XFS_I(inode);
+ struct xfs_mount *mp = ip->i_mount;
+ uint lock;
+ loff_t offset, end;
+ int error = 0;
+
+ if (XFS_FORCED_SHUTDOWN(mp))
+ return -EIO;
+
+ lock = xfs_ilock_data_map_shared(ip);
+
+ end = i_size_read(inode);
+ offset = __xfs_seek_hole_data(inode, start, end, whence);
+ if (offset < 0) {
+ error = offset;
+ goto out_unlock;
+ }
+
 offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
 

out_unlock:

fs/xfs/xfs_inode.h +2 -0

index ca9e119..ed7e933 100644
@@ -437,6 +437,8 @@ int xfs_update_prealloc_flags(struct xfs_inode *ip,
 int xfs_zero_eof(struct xfs_inode *ip, xfs_off_t offset,
 xfs_fsize_t isize, bool *did_zeroing);
 int xfs_iozero(struct xfs_inode *ip, loff_t pos, size_t count);
+loff_t __xfs_seek_hole_data(struct inode *inode, loff_t start,
+ loff_t eof, int whence);
 
 
 /* from xfs_iops.c */

Signature · 1 line

1.7.1

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 3 lines

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

So this is what I wanted to suggest at first as well. What I somewhat dislike is that 'addr' must now point to something like:

struct getquota_args {
	__u64 flags;
	struct fs_disk_quota ret;
};

which is not as nice as passing pointer to fs_disk_quota directly. But probably still OK. So I agree with the flags idea.

Another issue is that OCFS2 and ext4 with quota in hidden inodes would need call with these capabilities as well (they have the same problem as xfs). For reporting VFS quota we use bytes for space (since some filesystems need it) and we don't need RT fields (easy so zero-fill) and warning fields (there zeros may be confusing). So we would need something like struct qc_dqblk (currently only internal) extended with ID and maybe flags field XFS is using to return quota type which can contain all the information and tell which info is actually valid as return structure.

Honza

Quoted 15 lines · Jan Kara

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 3 lines

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

Quoted 7 lines

So this is what I wanted to suggest at first as well. What I somewhat dislike is that 'addr' must now point to something like:

struct getquota_args {
	__u64 flags;
	struct fs_disk_quota ret;
};

Ok...

Quoted 2 lines

which is not as nice as passing pointer to fs_disk_quota directly. But probably still OK. So I agree with the flags idea.

I understand that flags are for future-proofing, for possibly-not-yet-imagined scenarios, but on the other hand, I wonder how many different flavors of "get me a disk quota" we really expect to have?

Quoted 8 lines

Another issue is that OCFS2 and ext4 with quota in hidden inodes would need call with these capabilities as well (they have the same problem as xfs). For reporting VFS quota we use bytes for space (since some filesystems need it) and we don't need RT fields (easy so zero-fill) and warning fields (there zeros may be confusing). So we would need something like struct qc_dqblk (currently only internal) extended with ID and maybe flags field XFS is using to return quota type which can contain all the information and tell which info is actually valid as return structure.

Hohum, yeah - I forgot that ext4 has (can have?) a hidden quota inode. And I didn't know about OCFS2.

And TBH I did the xfs-specific one because it was trivial to extend, with the id already in the returned quota struct.

So ok, it sounds like this needs a more significant overhaul at the vfs quota level...

Quoted 48 lines · Eric Sandeen

Quoted 15 lines · Jan Kara

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 3 lines

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

Quoted 7 lines

So this is what I wanted to suggest at first as well. What I somewhat dislike is that 'addr' must now point to something like:

struct getquota_args {
	__u64 flags;
	struct fs_disk_quota ret;
};

Ok...

Quoted 2 lines

which is not as nice as passing pointer to fs_disk_quota directly. But probably still OK. So I agree with the flags idea.

I understand that flags are for future-proofing, for possibly-not-yet-imagined scenarios, but on the other hand, I wonder how many different flavors of "get me a disk quota" we really expect to have?

Quoted 8 lines

Another issue is that OCFS2 and ext4 with quota in hidden inodes would need call with these capabilities as well (they have the same problem as xfs). For reporting VFS quota we use bytes for space (since some filesystems need it) and we don't need RT fields (easy so zero-fill) and warning fields (there zeros may be confusing). So we would need something like struct qc_dqblk (currently only internal) extended with ID and maybe flags field XFS is using to return quota type which can contain all the information and tell which info is actually valid as return structure.

Hohum, yeah - I forgot that ext4 has (can have?) a hidden quota inode. And I didn't know about OCFS2.

And TBH I did the xfs-specific one because it was trivial to extend, with the id already in the returned quota struct.

Quoted 2 lines

So ok, it sounds like this needs a more significant overhaul at the vfs quota level...

Actually, what I want from you is just an interface which is usable for VFS quotas as well since I'd like to avoid adding GETQUOTA2 quotactl shortly after XGETQUOTA2 :).

Quoted 48 lines · Eric Sandeen

Quoted 15 lines · Jan Kara

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 3 lines

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

Quoted 7 lines

So this is what I wanted to suggest at first as well. What I somewhat dislike is that 'addr' must now point to something like:

struct getquota_args {
	__u64 flags;
	struct fs_disk_quota ret;
};

Ok...

Quoted 2 lines

which is not as nice as passing pointer to fs_disk_quota directly. But probably still OK. So I agree with the flags idea.

I understand that flags are for future-proofing, for possibly-not-yet-imagined scenarios, but on the other hand, I wonder how many different flavors of "get me a disk quota" we really expect to have?

Quoted 8 lines

Another issue is that OCFS2 and ext4 with quota in hidden inodes would need call with these capabilities as well (they have the same problem as xfs). For reporting VFS quota we use bytes for space (since some filesystems need it) and we don't need RT fields (easy so zero-fill) and warning fields (there zeros may be confusing). So we would need something like struct qc_dqblk (currently only internal) extended with ID and maybe flags field XFS is using to return quota type which can contain all the information and tell which info is actually valid as return structure.

Hohum, yeah - I forgot that ext4 has (can have?) a hidden quota inode. And I didn't know about OCFS2.

And TBH I did the xfs-specific one because it was trivial to extend, with the id already in the returned quota struct.

Quoted 2 lines

So ok, it sounds like this needs a more significant overhaul at the vfs quota level...

Actually, what I want from you is just an interface which is usable for VFS quotas as well since I'd like to avoid adding GETQUOTA2 quotactl shortly after XGETQUOTA2 :).

Quoted 49 lines · Jan Kara

Quoted 48 lines · Eric Sandeen

Quoted 15 lines · Jan Kara

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Eric Sandeen

This adds a new quotactl, Q_XGETQUOTA2.

Q_XGETQUOTA2 is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the specified id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned.

Quoted 3 lines

Please add a flags argument to Q_XGETQUOTA2, and then make the new behavior the first flag. Keep Q_XGETQUOTA behavior for the flag-less case. That way we get future etensibility for free.

Quoted 7 lines

So this is what I wanted to suggest at first as well. What I somewhat dislike is that 'addr' must now point to something like:

struct getquota_args {
	__u64 flags;
	struct fs_disk_quota ret;
};

Ok...

Quoted 2 lines

which is not as nice as passing pointer to fs_disk_quota directly. But probably still OK. So I agree with the flags idea.

I understand that flags are for future-proofing, for possibly-not-yet-imagined scenarios, but on the other hand, I wonder how many different flavors of "get me a disk quota" we really expect to have?

Quoted 8 lines

Another issue is that OCFS2 and ext4 with quota in hidden inodes would need call with these capabilities as well (they have the same problem as xfs). For reporting VFS quota we use bytes for space (since some filesystems need it) and we don't need RT fields (easy so zero-fill) and warning fields (there zeros may be confusing). So we would need something like struct qc_dqblk (currently only internal) extended with ID and maybe flags field XFS is using to return quota type which can contain all the information and tell which info is actually valid as return structure.

Hohum, yeah - I forgot that ext4 has (can have?) a hidden quota inode. And I didn't know about OCFS2.

And TBH I did the xfs-specific one because it was trivial to extend, with the id already in the returned quota struct.

Quoted 6 lines

Quoted 2 lines

So ok, it sounds like this needs a more significant overhaul at the vfs quota level...

Actually, what I want from you is just an interface which is usable for VFS quotas as well since I'd like to avoid adding GETQUOTA2 quotactl shortly after XGETQUOTA2 :).

Actually, that's exactly what I thought would *need* to happen ... we already have this weird 15-year-old split-brain quota interface, so if xfs and ext4 both need the same functionality, then we'd probably add both GETQUOTA2 and XGETQUOTA2. If we were doing this all from scratch, sure, but adding a new handles-both-quota-types interface when every other operation is already split between the two almost seems to make matters worse.

Quoted 1 line · Eric Sandeen

On 1/11/16 10:28 AM, Jan Kara wrote:

Quoted 10 lines

Quoted 3 lines

Actually, what I want from you is just an interface which is usable for VFS quotas as well since I'd like to avoid adding GETQUOTA2 quotactl shortly after XGETQUOTA2 :).

Actually, that's exactly what I thought would *need* to happen ... we already have this weird 15-year-old split-brain quota interface, so if xfs and ext4 both need the same functionality, then we'd probably add both GETQUOTA2 and XGETQUOTA2. If we were doing this all from scratch, sure, but adding a new handles-both-quota-types interface when every other operation is already split between the two almost seems to make matters worse.

Well, currently GETQUOTA and XGETQUOTA (and all the other quotactls) are actually translated so they work regardless of the underlying filesystem. So the only difference between XFS and VFS quotactls is in the formatting of input/output structures. So from kernel POV it seems somewhat pointless to add two calls doing the same thing and differing just in the formatting of output - especially when we want the call to be extensible.

I agree that having a unified call means having a new structure for passing dquot info between kernel and userspace. So just for adding that one small feature you want it seems like an overkill. But when thinking about new extensible getquota quotactl it IMHO makes sense to unify the VFS/XFS split brain. Thoughts?

My first lazy/hacky thought is "how terrible would it be to overload the quotactl syscall return value with quota ID for Q_GETQUOTA2 calls?"

Hi Jan -

Quoted 2 lines · Jan Kara

Quoted 1 line · Eric Sandeen

On 1/11/16 10:28 AM, Jan Kara wrote:

...

Quoted 18 lines

Quoted 10 lines

Quoted 3 lines

Actually, what I want from you is just an interface which is usable for VFS quotas as well since I'd like to avoid adding GETQUOTA2 quotactl shortly after XGETQUOTA2 :).

Actually, that's exactly what I thought would *need* to happen ... we already have this weird 15-year-old split-brain quota interface, so if xfs and ext4 both need the same functionality, then we'd probably add both GETQUOTA2 and XGETQUOTA2. If we were doing this all from scratch, sure, but adding a new handles-both-quota-types interface when every other operation is already split between the two almost seems to make matters worse.

Well, currently GETQUOTA and XGETQUOTA (and all the other quotactls) are actually translated so they work regardless of the underlying filesystem. So the only difference between XFS and VFS quotactls is in the formatting of input/output structures. So from kernel POV it seems somewhat pointless to add two calls doing the same thing and differing just in the formatting of output - especially when we want the call to be extensible.

Quoted 5 lines

I agree that having a unified call means having a new structure for passing dquot info between kernel and userspace. So just for adding that one small feature you want it seems like an overkill. But when thinking about new extensible getquota quotactl it IMHO makes sense to unify the VFS/XFS split brain. Thoughts?

My first lazy/hacky thought is "how terrible would it be to overload the quotactl syscall return value with quota ID for Q_GETQUOTA2 calls?"

For a purpose-built interface of "find the next ID" that wouldn't require any structure or interface changes...

We could name it Q_GETNEXTQUOTA / Q_XGETNEXTQUOTA to make it explicit about the purpose, and document that return behavior. Done & done. ;)

A new grand unified extensible quota call sounds like a great idea, I just hate to gate this work on designing a brand-new interface.

OK, ok. I like Dave's proposal for quotactl2(). So let's leave the unification for later and implent Q_GETNEXTQUOTA and Q_XGETNEXTQUOTA with the functionality of your original Q_XGETQUOTA2. Having separate call to get next ID would save us one new quotactl but OTOH we would need two syscalls (and quota structure lookups) to report one structure and there are potentially *lots* of them.

On Fri 15-01-16 11:31:00, Eric Sandeen wrote:

Quoted 8 lines

For a purpose-built interface of "find the next ID" that wouldn't require any structure or interface changes...

We could name it Q_GETNEXTQUOTA / Q_XGETNEXTQUOTA to make it explicit about the purpose, and document that return behavior. Done & done. ;)

A new grand unified extensible quota call sounds like a great idea, I just hate to gate this work on designing a brand-new interface.

OK, ok. I like Dave's proposal for quotactl2(). So let's leave the unification for later and implent Q_GETNEXTQUOTA and Q_XGETNEXTQUOTA with the functionality of your original Q_XGETQUOTA2. Having separate call to get next ID would save us one new quotactl but OTOH we would need two syscalls (and quota structure lookups) to report one structure and there are potentially *lots* of them.

Quoted 1 line · Jan Kara

On Fri 15-01-16 11:31:00, Eric Sandeen wrote:

...

Quoted 9 lines

Quoted 8 lines

For a purpose-built interface of "find the next ID" that wouldn't require any structure or interface changes...

We could name it Q_GETNEXTQUOTA / Q_XGETNEXTQUOTA to make it explicit about the purpose, and document that return behavior. Done & done. ;)

A new grand unified extensible quota call sounds like a great idea, I just hate to gate this work on designing a brand-new interface.

Quoted 6 lines

OK, ok. I like Dave's proposal for quotactl2(). So let's leave the unification for later and implent Q_GETNEXTQUOTA and Q_XGETNEXTQUOTA with the functionality of your original Q_XGETQUOTA2. Having separate call to get next ID would save us one new quotactl but OTOH we would need two syscalls (and quota structure lookups) to report one structure and there are potentially *lots* of them.

Ok, I can re-do it with the new Q_[X]GETNEXTQUOTA names, I've already done the non-xfs one as well, just starting testing on that.

With or without the flags argument?

Quoted 21 lines · Eric Sandeen

Quoted 1 line · Jan Kara

On Fri 15-01-16 11:31:00, Eric Sandeen wrote:

...

Quoted 15 lines

Quoted 8 lines

For a purpose-built interface of "find the next ID" that wouldn't require any structure or interface changes...

We could name it Q_GETNEXTQUOTA / Q_XGETNEXTQUOTA to make it explicit about the purpose, and document that return behavior. Done & done. ;)

A new grand unified extensible quota call sounds like a great idea, I just hate to gate this work on designing a brand-new interface.

OK, ok. I like Dave's proposal for quotactl2(). So let's leave the unification for later and implent Q_GETNEXTQUOTA and Q_XGETNEXTQUOTA with the functionality of your original Q_XGETQUOTA2. Having separate call to get next ID would save us one new quotactl but OTOH we would need two syscalls (and quota structure lookups) to report one structure and there are potentially *lots* of them.

Quoted 4 lines

Ok, I can re-do it with the new Q_[X]GETNEXTQUOTA names, I've already done the non-xfs one as well, just starting testing on that.

With or without the flags argument?

Without. For further extensibility I'd really go for the unified API in the end.

Quoted 6 lines · Eric Sandeen

Actually, that's exactly what I thought would *need* to happen ... we already have this weird 15-year-old split-brain quota interface, so if xfs and ext4 both need the same functionality, then we'd probably add both GETQUOTA2 and XGETQUOTA2. If we were doing this all from scratch, sure, but adding a new handles-both-quota-types interface when every other operation is already split between the two almost seems to make matters worse.

Well, currently GETQUOTA and XGETQUOTA (and all the other quotactls) are actually translated so they work regardless of the underlying filesystem. So the only difference between XFS and VFS quotactls is in the formatting of input/output structures. So from kernel POV it seems somewhat pointless to add two calls doing the same thing and differing just in the formatting of output - especially when we want the call to be extensible.

I agree that having a unified call means having a new structure for passing dquot info between kernel and userspace. So just for adding that one small feature you want it seems like an overkill. But when thinking about new extensible getquota quotactl it IMHO makes sense to unify the VFS/XFS split brain. Thoughts?