Linux 4.11 compat getattr/struct kstat changes #5875

ofaaland · 2017-03-07T22:51:20Z

Description

Accomodate changes to struct kstat and getattr-related interfaces. See also the related SPL changes.
Requires-spl: refs/pull/608/head

Motivation and Context

Linux 4.11 struct kstat and getattr-related interface changes breaks build.

How Has This Been Tested?

Built and tested with ztest on x86_64 RHEL7 VM. Depending on builbot for more complete testing.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
Change has been approved by a ZFS on Linux member.

mention-bot · 2017-03-07T22:51:21Z

@ofaaland, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gmelikov, @dajhorn and @behlendorf to be potential reviewers.

tuxoko · 2017-03-08T02:03:35Z

@ofaaland
iops->getattr and related stuffs are also changed

ofaaland · 2017-03-08T02:22:10Z

Yep.

ofaaland · 2017-03-09T00:38:07Z

This first crack at handling the iops->getattr and simple_getattr() changes is mostly just to get it to build and ensure the configure checks are working.

tuxoko · 2017-03-09T19:25:58Z

config/kernel-inode-getattr.m4

+		#include <linux/fs.h>
+
+		int test_getattr(
+		    const struct path *p, struct dentry *d, struct kstat *k,


There's no dentry in the new form.

tuxoko · 2017-03-09T19:26:57Z

module/zfs/zpl_ctldir.c

+    u32 request_mask,
+    unsigned int query_flags
+#endif /* defined(HAVE_PATH_SIMPLE_GETATTR) */
+)


You should use new form for this function, and make a wrapper function with old form inside ifdef

@tuxoko,
Thanks for the feedback and pointing out the dentry error.

I'm not certain I understand what you mean. Using this function as an example, it must provide the same interface as simple_getattr() and also calls simple_getattr() within its body, so it seems to me that one either has conditional blocks like this, or separate implementations so that the arguments received can be passed on to the simple_getattr() call.

That said, I agree this is not easy to read and likely not the best way. Brian pointed out that in some cases I could move the common logic into an _impl function and then have one version with the old interface and one with the new, wrapped in the appropriate ifdefs, that both call the same common function after making the simple_getattr() call appropriately.

I just pushed an update that fixes the mistakes that earlier were preventing it from building. I'll work on it more tomorrow.

tuxoko · 2017-03-10T23:30:22Z

diff --git a/include/linux/vfs_compat.h b/include/linux/vfs_compat.h
index baa2980..e093e46 100644
--- a/include/linux/vfs_compat.h
+++ b/include/linux/vfs_compat.h
@@ -454,4 +454,26 @@ setattr_prepare(struct dentry *dentry, struct iattr *ia)
 }
 #endif
 
+#ifdef HAVE_3ARGS_IOPS_GETATTR
+#define	GETATTR_WRAPPER(func)						\
+static int								\
+func(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)	\
+{									\
+	struct path = { .mnt = mnt, .dentry = dentry };			\
+	return func##_impl(&path, stat, STATX_BASIC_STATS,		\
+	    AT_STATX_SYNC_AS_STAT);					\
+}
+#elif defined(HAVE_4ARGS_IOPS_GETATTR)
+#define	GETATTR_WRAPPER(func)						\
+static int								\
+func(struct path *path, struct kstat *stat, u32 request_mask,		\
+    unsigned int query_flags)						\
+{									\
+	return func##_impl(path, stat, request_mask, query_flags);	\
+}
+#else
+#error
+#endif
+
+
 #endif /* _ZFS_VFS_H */
diff --git a/module/zfs/zpl_ctldir.c b/module/zfs/zpl_ctldir.c
index cdd6668..18ca1d4 100644
--- a/module/zfs/zpl_ctldir.c
+++ b/module/zfs/zpl_ctldir.c
@@ -100,16 +100,21 @@ zpl_root_readdir(struct file *filp, void *dirent, filldir_t filldir)
  */
 /* ARGSUSED */
 static int
-zpl_root_getattr(struct vfsmount *mnt, struct dentry *dentry,
-    struct kstat *stat)
+zpl_root_getattr_impl(struct path *path, struct kstat *stat, u32 request_mask,
+    unsigned int query_flags)
 {
 	int error;
 
-	error = simple_getattr(mnt, dentry, stat);
+#ifdef HAVE_3ARGS_IOPS_GETATTR
+	error = simple_getattr(path->mnt, path->dentry, stat);
+#else
+	error = simple_getattr(path, stat, request_mask, query_flags);
+#endif
 	stat->atime = CURRENT_TIME;
 
 	return (error);
 }
+GETATTR_WRAPPER(zpl_root_getattr);
 
 static struct dentry *
 #ifdef HAVE_LOOKUP_NAMEIDATA
@@ -375,14 +380,18 @@ zpl_snapdir_mkdir(struct inode *dip, struct dentry *dentry, zpl_umode_t mode)
  */
 /* ARGSUSED */
 static int
-zpl_snapdir_getattr(struct vfsmount *mnt, struct dentry *dentry,
-    struct kstat *stat)
+zpl_snapdir_getattr_impl(struct path *path, struct kstat *stat, u32 request_mask,
+    unsigned int query_flags)
 {
 	zfs_sb_t *zsb = ITOZSB(dentry->d_inode);
 	int error;
 
 	ZFS_ENTER(zsb);
-	error = simple_getattr(mnt, dentry, stat);
+#ifdef HAVE_3ARGS_IOPS_GETATTR
+	error = simple_getattr(path->mnt, path->dentry, stat);
+#else
+	error = simple_getattr(path, stat, request_mask, query_flags);
+#endif
 	stat->nlink = stat->size = 2;
 	stat->ctime = stat->mtime = dmu_objset_snap_cmtime(zsb->z_os);
 	stat->atime = CURRENT_TIME;
@@ -390,6 +399,7 @@ zpl_snapdir_getattr(struct vfsmount *mnt, struct dentry *dentry,
 
 	return (error);
 }
+GETATTR_WRAPPER(zpl_root_getattr);
 
 /*
  * The '.zfs/snapshot' directory file operations.  These mainly control
diff --git a/module/zfs/zpl_inode.c b/module/zfs/zpl_inode.c
index b39a8bb..dc6af11 100644
--- a/module/zfs/zpl_inode.c
+++ b/module/zfs/zpl_inode.c
@@ -340,18 +340,20 @@ zpl_rmdir(struct inode *dir, struct dentry *dentry)
 }
 
 static int
-zpl_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
+zpl_getattr_impl(struct path *path, struct kstat *stat, u32 request_mask,
+    unsigned int query_flags)
 {
 	int error;
 	fstrans_cookie_t cookie;
 
 	cookie = spl_fstrans_mark();
-	error = -zfs_getattr_fast(dentry->d_inode, stat);
+	error = -zfs_getattr_fast(path->dentry->d_inode, stat);
 	spl_fstrans_unmark(cookie);
 	ASSERT3S(error, <=, 0);
 
 	return (error);
 }
+GETATTR_WRAPPER(zpl_getattr);
 
 static int
 zpl_setattr(struct dentry *dentry, struct iattr *ia)

@ofaaland
Can you try something like this.

ofaaland · 2017-03-14T02:29:42Z

@tuxoko, I see. I got sidetracked but will come back to this. I personally dislike using token pasting with function names because I depend a lot on tagging around. But it does clean up some of the mess and make the new interface stand out a lot better. I'll play with it more tomorrow.

behlendorf · 2017-03-14T17:25:24Z

We do something similar to this already in zpl_xattr.c. I think this could be OK if we also provided a wrapper for simple_getattr(). Also if we do go this route lets call the wrapper macro ZPL_GETATTR_WRAPPER which is consistent with the xattr version.

Made the suggested change

tuxoko · 2017-03-16T19:19:15Z

module/zfs/zpl_ctldir.c

+ * Used by zpl*getattr* functions below.
+ */
+static int
+zpl_simple_getattr(const struct path *path, struct kstat *stat,


I think we should get rid of this function and just call generic_fillattr. That'll make things much easier.
The only difference is simple_getattr stat->blocks = inode->i_mapping->nrpages << (PAGE_SHIFT - 9); which we don't really use since the caller are all directories.

tuxoko · 2017-03-16T19:22:17Z

module/zfs/zfs_vnops.c

+	/* Must be set before the call to generic_fillattr() */
+	stat->result_mask = STATX_BASIC_STATS;
+#endif /* defined(HAVE_STAT_RESULT_MASK) */
+


I don't think we need this chunk, this is already done in vfs layer.
http://elixir.free-electrons.com/source/fs/stat.c?v=4.11-rc2#L67

It's done by vfs_getattr(), but not by simple_getattr() or generic_fillattr().

Yes, and this function and simple_getattr are both hooks for iop->getattr(), so we don't need to do it if simple_getattr() don't do it.

Made the requested changes.

behlendorf · 2017-03-20T18:29:14Z

module/zfs/zpl_ctldir.c

-	int error;
-
-	error = simple_getattr(mnt, dentry, stat);
+	generic_fillattr(path->dentry->d_inode, stat);


Switching to generic_fillattr() means we're dropping the blocks update. However, that shouldn't be an issue since these are all virtual directories anyway.

generic_fillattr does set blocks from i_blocks. simple_getattr set blocks according to mmapped pages which only makes sense for tmpfs.

behlendorf · 2017-03-20T18:35:50Z

config/kernel-struct-kstat.m4

+	],[
+		AC_MSG_RESULT(no)
+	])
+])


This check is entirely unused. Did you intend to use it in the actual patch?

It was used for code that got removed. I'll remove it and push again.

behlendorf · 2017-03-20T20:38:17Z

Almost there, one remaining fix.

configure:19136: error: possibly undefined macro: ZFS_AC_KERNEL_STAT_RESULT_MASK

In torvalds/linux@a528d35, there are changes to the getattr family of functions, struct kstat, and the interface of inode_operations .getattr. The inode_operations .getattr and simple_getattr() interface changed to: int (*getattr) (const struct path *, struct dentry *, struct kstat *, u32 request_mask, unsigned int query_flags) The request_mask argument indicates which field(s) the caller intends to use. Fields the caller has not specified via request_mask may be set in the returned struct anyway, but their values may be approximate. The query_flags argument indicates whether the filesystem must update the attributes from the backing store. Currently both fields are ignored. It is possible that getattr-related functions within zfs could be optimized based on the request_mask. struct kstat includes new fields: u32 result_mask; /* What fields the user got */ u64 attributes; /* See STATX_ATTR_* flags */ struct timespec btime; /* File creation time */ Fields attribute and btime are cleared; the result_mask reflects this. These appear to be optional based on simple_getattr() and vfs_getattr() within the kernel, which take the same approach. Requires-spl: refs/pull/608/head Signed-off-by: Olaf Faaland <faaland1@llnl.gov>

behlendorf · 2017-03-21T00:51:40Z

@ofaaland @tuxoko thanks! It's great to have this fixed!

In torvalds/linux@a528d35, there are changes to the getattr family of functions, struct kstat, and the interface of inode_operations .getattr. The inode_operations .getattr and simple_getattr() interface changed to: int (*getattr) (const struct path *, struct dentry *, struct kstat *, u32 request_mask, unsigned int query_flags) The request_mask argument indicates which field(s) the caller intends to use. Fields the caller has not specified via request_mask may be set in the returned struct anyway, but their values may be approximate. The query_flags argument indicates whether the filesystem must update the attributes from the backing store. Currently both fields are ignored. It is possible that getattr-related functions within zfs could be optimized based on the request_mask. struct kstat includes new fields: u32 result_mask; /* What fields the user got */ u64 attributes; /* See STATX_ATTR_* flags */ struct timespec btime; /* File creation time */ Fields attribute and btime are cleared; the result_mask reflects this. These appear to be optional based on simple_getattr() and vfs_getattr() within the kernel, which take the same approach. Reviewed-by: Chunwei Chen <david.chen@osnexus.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Olaf Faaland <faaland1@llnl.gov> Closes openzfs#5875

jgottula · 2017-05-23T06:29:05Z

I'm reasonably certain that this 4.11 compat patch (a3478c0) is responsible for a new(ish) bug related to automounting in .zfs/snapshot: #6154

svde · 2017-06-02T06:50:45Z

Fedora 25 was updated to 4.11.3-200.fc25. This bug is preventing the dkms module from building.

In torvalds/linux@a528d35, there are changes to the getattr family of functions, struct kstat, and the interface of inode_operations .getattr. The inode_operations .getattr and simple_getattr() interface changed to: int (*getattr) (const struct path *, struct dentry *, struct kstat *, u32 request_mask, unsigned int query_flags) The request_mask argument indicates which field(s) the caller intends to use. Fields the caller has not specified via request_mask may be set in the returned struct anyway, but their values may be approximate. The query_flags argument indicates whether the filesystem must update the attributes from the backing store. Currently both fields are ignored. It is possible that getattr-related functions within zfs could be optimized based on the request_mask. struct kstat includes new fields: u32 result_mask; /* What fields the user got */ u64 attributes; /* See STATX_ATTR_* flags */ struct timespec btime; /* File creation time */ Fields attribute and btime are cleared; the result_mask reflects this. These appear to be optional based on simple_getattr() and vfs_getattr() within the kernel, which take the same approach. Reviewed-by: Chunwei Chen <david.chen@osnexus.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Olaf Faaland <faaland1@llnl.gov> Closes #5875

ofaaland force-pushed the b_vfs_getattr branch from ea9405b to 3b40732 Compare March 7, 2017 23:43

ofaaland added Type: Building Indicates an issue related to building binaries Status: Work in Progress Not yet ready for general review labels Mar 7, 2017

ofaaland force-pushed the b_vfs_getattr branch 2 times, most recently from c77fc03 to d29eda8 Compare March 8, 2017 00:45

ofaaland force-pushed the b_vfs_getattr branch from d29eda8 to 9a87757 Compare March 9, 2017 00:32

ofaaland force-pushed the b_vfs_getattr branch 4 times, most recently from d8ef1d3 to bec4989 Compare March 9, 2017 01:41

tuxoko previously requested changes Mar 9, 2017

View reviewed changes

ofaaland force-pushed the b_vfs_getattr branch from beb577a to 2d77947 Compare March 10, 2017 02:07

ofaaland force-pushed the b_vfs_getattr branch from 2d77947 to b4ae79f Compare March 14, 2017 02:21

ofaaland force-pushed the b_vfs_getattr branch 5 times, most recently from 8f86252 to 73e8c64 Compare March 16, 2017 02:06

ofaaland force-pushed the b_vfs_getattr branch from 73e8c64 to 63912fe Compare March 16, 2017 18:40

tuxoko suggested changes Mar 16, 2017

View reviewed changes

tuxoko previously requested changes Mar 16, 2017

View reviewed changes

ofaaland force-pushed the b_vfs_getattr branch from 63912fe to 9e4264c Compare March 16, 2017 19:58

ofaaland force-pushed the b_vfs_getattr branch from 9e4264c to 5de8aaf Compare March 16, 2017 21:08

tuxoko approved these changes Mar 16, 2017

View reviewed changes

ofaaland removed the Status: Work in Progress Not yet ready for general review label Mar 16, 2017

ofaaland changed the title ~~[WIP] SPL PR #608 for Linux 4.11 compat vfs_getattr() change~~ SPL PR #608 for Linux 4.11 compat vfs_getattr() change Mar 16, 2017

ofaaland changed the title ~~SPL PR #608 for Linux 4.11 compat vfs_getattr() change~~ SPL PR #608 for Linux 4.11 compat getattr/struct kstat changes Mar 16, 2017

ofaaland changed the title ~~SPL PR #608 for Linux 4.11 compat getattr/struct kstat changes~~ Linux 4.11 compat getattr/struct kstat changes Mar 16, 2017

behlendorf approved these changes Mar 20, 2017

View reviewed changes

behlendorf requested changes Mar 20, 2017

View reviewed changes

ofaaland force-pushed the b_vfs_getattr branch from e8fa1a1 to 6af56b2 Compare March 20, 2017 20:03

ofaaland force-pushed the b_vfs_getattr branch from 6af56b2 to 711d379 Compare March 20, 2017 21:00

behlendorf approved these changes Mar 20, 2017

View reviewed changes

behlendorf merged commit a3478c0 into openzfs:master Mar 21, 2017

ofaaland deleted the b_vfs_getattr branch April 13, 2017 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux 4.11 compat getattr/struct kstat changes #5875

Linux 4.11 compat getattr/struct kstat changes #5875

ofaaland commented Mar 7, 2017 •

edited

Loading

mention-bot commented Mar 7, 2017

tuxoko commented Mar 8, 2017

ofaaland commented Mar 8, 2017

ofaaland commented Mar 9, 2017

tuxoko Mar 9, 2017

tuxoko Mar 9, 2017

ofaaland Mar 10, 2017

tuxoko commented Mar 10, 2017

ofaaland commented Mar 14, 2017

behlendorf commented Mar 14, 2017

tuxoko Mar 16, 2017

ofaaland Mar 16, 2017

tuxoko Mar 16, 2017

ofaaland Mar 16, 2017

tuxoko Mar 16, 2017

ofaaland Mar 16, 2017

behlendorf Mar 20, 2017

tuxoko Mar 20, 2017

behlendorf Mar 20, 2017

behlendorf Mar 20, 2017

ofaaland Mar 20, 2017

behlendorf commented Mar 20, 2017

behlendorf commented Mar 21, 2017

jgottula commented May 23, 2017

svde commented Jun 2, 2017

Linux 4.11 compat getattr/struct kstat changes #5875

Linux 4.11 compat getattr/struct kstat changes #5875

Conversation

ofaaland commented Mar 7, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Mar 7, 2017

tuxoko commented Mar 8, 2017

ofaaland commented Mar 8, 2017

ofaaland commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxoko commented Mar 10, 2017

ofaaland commented Mar 14, 2017

behlendorf commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf commented Mar 20, 2017

behlendorf commented Mar 21, 2017

jgottula commented May 23, 2017

svde commented Jun 2, 2017

ofaaland commented Mar 7, 2017 •

edited

Loading