xfs: use xfs_eofblocks_internal for eofblocks scanning
RFC PATCH 1/1 xfs: use xfs_eofblocks_internal for eofblocks scanning
Adds support for additional internal-only functionality to eofblocks scans such as the scan_owner field. The scan owner defines an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eof blocks.
---
Hi Dwight,
Here's that patch I referred to... as you said, it's not really that complicated, so take it or leave it. I suspect you'd have to drop the eof_scan_owner bits (that's for my use case that is not upstream yet), change over the uid/gid types, move the defs from xfs_fs.h into xfs_icache.h (I seem to have got this wrong as well), then pull the conversion up into the ioctl code rather than xfs_icache.c. It might just be easier to modify what you have already...
Brian
fs/xfs/xfs_fs.h | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_icache.c | 43 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 8 deletions(-)
fs/xfs/xfs_fs.h +26 -0
@@ -370,6 +370,32 @@ struct xfs_eofblocks { XFS_EOF_FLAGS_MINFILESIZE | \ XFS_EOF_FLAGS_FLUSH) +struct xfs_eofblocks_internal { +__u32 eof_version; +__u32 eof_flags; +uid_t eof_uid; +gid_t eof_gid; +prid_t eof_prid; +__u64 eof_min_file_size; +xfs_ino_t eof_scan_owner; +}; + +static inline void +xfs_eofb_to_internal ( +struct xfs_eofblocks_internal *ei, +struct xfs_eofblocks *e, +xfs_ino_t ino) +{ + ei->eof_version = e->eof_version; + ei->eof_flags = e->eof_flags; + ei->eof_uid = e->eof_uid; + ei->eof_gid = e->eof_gid; + ei->eof_prid = e->eof_prid; + ei->eof_min_file_size = e->eof_min_file_size; + + ei->eof_scan_owner = ino; +} +/* * The user-level Handle Request interface structure.
fs/xfs/xfs_icache.c +35 -8
79 lines not on this page · raw message
Signature · 6 lines
1.8.1.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 49 lines · Brian Foster
Adds support for additional internal-only functionality to eofblocks scans such as the
scan_ownerfield. The scan owner defines an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eof blocks.Signed-off-by Brian Foster <[email protected]>---
Hi Dwight,
Here's that patch I referred to... as you said, it's not really that complicated, so take it or leave it. I suspect you'd have to drop the
eof_scan_ownerbits (that's for my use case that is not upstream yet), change over the uid/gid types, move the defs fromxfs_fs.hintoxfs_icache.h(I seem to have got this wrong as well), then pull the conversion up into the ioctl code rather thanxfs_icache.c. It might just be easier to modify what you have already...Brian
fs/xfs/xfs_fs.h | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_icache.c | 43 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 8 deletions(-)fs/xfs/xfs_fs.h +15
-0@@ -370,6 +370,32 @@ struct xfs_eofblocks { XFS_EOF_FLAGS_MINFILESIZE | \ XFS_EOF_FLAGS_FLUSH) +struct xfs_eofblocks_internal { +__u32 eof_version; +__u32 eof_flags; +uid_t eof_uid; +gid_t eof_gid; +prid_t eof_prid; +__u64 eof_min_file_size; +xfs_ino_t eof_scan_owner; +}; + +static inline void +xfs_eofb_to_internal ( +struct xfs_eofblocks_internal *ei, +struct xfs_eofblocks *e, +xfs_ino_t ino)
fs/xfs/xfs_fs.h struct xfs_eofblocks { · line 397
Quoted 10 lines
fs/xfs/xfs_fs.h +10
-0+{ + ei->eof_version = e->eof_version; + ei->eof_flags = e->eof_flags; + ei->eof_uid = e->eof_uid; + ei->eof_gid = e->eof_gid; + ei->eof_prid = e->eof_prid; + ei->eof_min_file_size = e->eof_min_file_size; + + ei->eof_scan_owner = ino; +}
This doesn't belong in xfs_fs.h. xfs_fs.h defines userspace access interfaces, not internal kernel-only structures.
As it is, I really don't like the name xfs_eofblocks_internal. I'd prefer the userspace structure uses the prefix "xfs_fs_...." and the internal, kernel only one drops the "_fs" part of the name. i.e. the kernel code doesn't need to change at all, only the name of the external structure.
Quoted 5 lines
fs/xfs/xfs_fs.h +1
-0@@ -1244,6 +1245,16 @@ xfs_inode_free_eofblocks(if (eofb->eof_flags &XFS_EOF_FLAGS_FLUSH )filemap_flush (VFS_I (ip)->i_mapping); +
fs/xfs/xfs_fs.h xfs_inode_free_eofblocks( · line 1258
Quoted 10 lines
fs/xfs/xfs_fs.h +9
-0+/* +* A scan owner implies we already hold the iolock. Skip it in +* xfs_free_eofblocks() to avoid deadlock. This also eliminates +* the possibility of EAGAIN being returned. +*/ +if (eofb->eof_scan_owner !=NULLFSINO && + eofb->eof_scan_owner == ip->i_ino) + need_iolock = false; + }
That's a separate fix, right? So a separate patch?
And, FWIW, by definition ip->i_ino is never NULLFSINO by the time it is inserted into the cache, and so the only check needed is if(eofb->eof_scan_owner == ip->i_ino)....
Quoted 35 lines
fs/xfs/xfs_fs.h +10
-5/* @@ -1254,7 +1265,7 @@ xfs_inode_free_eofblocks( mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))return 0 ; - ret =xfs_free_eofblocks (ip->i_mount, ip, true); + ret =xfs_free_eofblocks (ip->i_mount, ip, need_iolock);/* don't revisit the inode if we're not waiting */ if (ret ==EAGAIN && !(flags &SYNC_WAIT ))@@ -1263,10 +1274,10 @@ xfs_inode_free_eofblocks(return ret; } -int -xfs_icache_free_eofblocks ( -struct xfs_mount *mp, -struct xfs_eofblocks *eofb) +STATIC int +__xfs_icache_free_eofblocks ( +struct xfs_mount *mp, +struct xfs_eofblocks_internal *eofb) {int flags =SYNC_TRYLOCK ;@@ -1277,6 +1288,22 @@ xfs_icache_free_eofblocks( eofb, XFS_ICI_EOFBLOCKS_TAG); } +int +xfs_icache_free_eofblocks ( +struct xfs_mount *mp, +struct xfs_eofblocks *eofb) +{
fs/xfs/xfs_fs.h xfs_icache_free_eofblocks( · line 1305
Quoted 10 lines
fs/xfs/xfs_fs.h +10
-0+struct xfs_eofblocks_internal eofb_i; +struct xfs_eofblocks_internal *eofb_p =NULL ; + +if (eofb) { +xfs_eofb_to_internal (&eofb_i, eofb,NULLFSINO ); + eofb_p = &eofb_i; + } + +return __xfs_icache_free_eofblocks (mp, eofb_p); +}
This conversion should be done in the xfs_ioctl.c - that's the only place where the "external" version of the structure is used, and that's where we convert to "internal" kernel only structures for calls into the kernel core code. All kernel internal users shoul dbe using the kernel-only structure definition directly....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 55 lines · Dave Chinner
Quoted 54 lines · Brian Foster
Adds support for additional internal-only functionality to eofblocks scans such as the
scan_ownerfield. The scan owner defines an optional inode number that is responsible for the current scan. The purpose is to identify that an inode is under iolock and as such, the iolock shouldn't be attempted when trimming eof blocks.Signed-off-by Brian Foster <[email protected]>---
Hi Dwight,
Here's that patch I referred to... as you said, it's not really that complicated, so take it or leave it. I suspect you'd have to drop the
eof_scan_ownerbits (that's for my use case that is not upstream yet), change over the uid/gid types, move the defs fromxfs_fs.hintoxfs_icache.h(I seem to have got this wrong as well), then pull the conversion up into the ioctl code rather thanxfs_icache.c. It might just be easier to modify what you have already...Brian
fs/xfs/xfs_fs.h | 26 ++++++++++++++++++++++++++ fs/xfs/xfs_icache.c | 43 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 8 deletions(-)fs/xfs/xfs_fs.h +20
-0@@ -370,6 +370,32 @@ struct xfs_eofblocks { XFS_EOF_FLAGS_MINFILESIZE | \ XFS_EOF_FLAGS_FLUSH) +struct xfs_eofblocks_internal { +__u32 eof_version; +__u32 eof_flags; +uid_t eof_uid; +gid_t eof_gid; +prid_t eof_prid; +__u64 eof_min_file_size; +xfs_ino_t eof_scan_owner; +}; + +static inline void +xfs_eofb_to_internal ( +struct xfs_eofblocks_internal *ei, +struct xfs_eofblocks *e, +xfs_ino_t ino) +{ + ei->eof_version = e->eof_version; + ei->eof_flags = e->eof_flags; + ei->eof_uid = e->eof_uid; + ei->eof_gid = e->eof_gid;
Quoted 9 lines
fs/xfs/xfs_fs.h struct xfs_eofblocks { · line 397
Quoted 5 lines
fs/xfs/xfs_fs.h +5
-0+ ei->eof_prid = e->eof_prid; + ei->eof_min_file_size = e->eof_min_file_size; + + ei->eof_scan_owner = ino; +}This doesn't belong in
xfs_fs.h.xfs_fs.hdefines userspace access interfaces, not internal kernel-only structures.
Yeah, I sent this along out of my tree as an FYI for Dwight, knowing that it wasn't quite ready yet. FWIW, I noted the incorrect header above... ;)
Quoted 6 lines
As it is, I really don't like the name
xfs_eofblocks_internal. I'd prefer the userspace structure uses the prefix "xfs_fs_...." and the internal, kernel only one drops the "_fs" part of the name. i.e. the kernel code doesn't need to change at all, only the name of the external structure.
Ok, so you are suggesting we actually change the exported structure name? I ask because I mentioned to Dwight previously that we didn't want to go and change xfs_eofblocks to xfs_ueofblocks, considering it was exported. Not that there are many users...
I'm Ok with the name change if there's no issue changing it after the fact. xfs_fs_eofblocks for the exported structure and xfs_eofblocks for the internal one sounds fine to me. (Dwight, sorry for the 180 on that).
Quoted 18 lines
Quoted 5 lines
fs/xfs/xfs_fs.h +1
-0@@ -1244,6 +1245,16 @@ xfs_inode_free_eofblocks(if (eofb->eof_flags &XFS_EOF_FLAGS_FLUSH )filemap_flush (VFS_I (ip)->i_mapping); +fs/xfs/xfs_fs.h xfs_inode_free_eofblocks( · line 1258
Quoted 10 lines
fs/xfs/xfs_fs.h +9
-0+/* +* A scan owner implies we already hold the iolock. Skip it in +* xfs_free_eofblocks() to avoid deadlock. This also eliminates +* the possibility of EAGAIN being returned. +*/ +if (eofb->eof_scan_owner !=NULLFSINO && + eofb->eof_scan_owner == ip->i_ino) + need_iolock = false; + }That's a separate fix, right? So a separate patch?
I noticed that as well. That should be pulled out separately. It most likely will be now that the structure separation may occur for the userns use case first.
Quoted 4 lines
And, FWIW, by definition
ip->i_inois never NULLFSINO by the time it is inserted into the cache, and so the only check needed isif(eofb->eof_scan_owner == ip->i_ino)....
Ok, good point.
Quoted 44 lines
Quoted 44 lines
fs/xfs/xfs_fs.h +18
-5/* @@ -1254,7 +1265,7 @@ xfs_inode_free_eofblocks( mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY))return 0 ; - ret =xfs_free_eofblocks (ip->i_mount, ip, true); + ret =xfs_free_eofblocks (ip->i_mount, ip, need_iolock);/* don't revisit the inode if we're not waiting */ if (ret ==EAGAIN && !(flags &SYNC_WAIT ))@@ -1263,10 +1274,10 @@ xfs_inode_free_eofblocks(return ret; } -int -xfs_icache_free_eofblocks ( -struct xfs_mount *mp, -struct xfs_eofblocks *eofb) +STATIC int +__xfs_icache_free_eofblocks ( +struct xfs_mount *mp, +struct xfs_eofblocks_internal *eofb) {int flags =SYNC_TRYLOCK ;@@ -1277,6 +1288,22 @@ xfs_icache_free_eofblocks( eofb, XFS_ICI_EOFBLOCKS_TAG); } +int +xfs_icache_free_eofblocks ( +struct xfs_mount *mp, +struct xfs_eofblocks *eofb) +{ +struct xfs_eofblocks_internal eofb_i; +struct xfs_eofblocks_internal *eofb_p =NULL ; + +if (eofb) { +xfs_eofb_to_internal (&eofb_i, eofb,NULLFSINO ); + eofb_p = &eofb_i; + } +
Quoted 9 lines
fs/xfs/xfs_fs.h xfs_icache_free_eofblocks( · line 1305
Quoted 2 lines
fs/xfs/xfs_fs.h +2
-0+return __xfs_icache_free_eofblocks (mp, eofb_p); +}This conversion should be done in the
xfs_ioctl.c- that's the only place where the "external" version of the structure is used, and that's where we convert to "internal" kernel only structures for calls into the kernel core code. All kernel internal users shoul dbe using the kernel-only structure definition directly....
This is the approach Dwight is going to take (re: the "userns: Convert xfs to use kuid/kgid where appropriate" thread), I believe. Thanks Dave.
Brian
Quoted 4 lines
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs