[PATCHSET RFC] support RESOLVE_CACHED for statx

[PATCHSET RFC] support RESOLVE_CACHED for statx

[Jens Axboe]

Hi,

This is a followup to the RESOLVE_CACHED addition that allows us to
speedup the io_uring open side (and enable RESOLVE_CACHED through
openat2). Mostly straight forward, as you can see from patch 1, this
just adds AT_STATX_CACHED that sits on top of that. Patch 2 is the
mostly ugly part, but not sure how we can do this any better - we need
to ensure that any sort of revalidation or sync in ->getattr() honors
it too. Patch 3 is just adapting to this in io_uring.

 fs/9p/vfs_inode.c          |  2 ++
 fs/afs/inode.c             |  3 +++
 fs/ceph/inode.c            |  2 ++
 fs/cifs/inode.c            |  3 +++
 fs/coda/inode.c            |  7 ++++++-
 fs/ecryptfs/inode.c        |  3 +++
 fs/fuse/dir.c              |  2 ++
 fs/gfs2/inode.c            |  2 ++
 fs/io_uring.c              | 21 ++++++++++++++-------
 fs/kernfs/inode.c          |  8 +++++++-
 fs/nfs/inode.c             |  3 +++
 fs/ocfs2/file.c            |  3 +++
 fs/orangefs/inode.c        |  3 +++
 fs/stat.c                  |  4 +++-
 fs/ubifs/dir.c             |  7 ++++++-
 fs/udf/symlink.c           |  3 +++
 fs/vboxsf/utils.c          |  4 ++++
 include/uapi/linux/fcntl.h |  2 ++
 18 files changed, 71 insertions(+), 11 deletions(-)

-- 
Jens Axboe

[PATCH 1/3] fs: add support for AT_STATX_CACHED

[Jens Axboe]

Since we now have LOOKUP_CACHED to only perform a fast lookup of the
dentry cache for path resolution, expose this as AT_STATX_CACHED to
provide the same functionality on the statx side.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/stat.c                  | 4 +++-
 include/uapi/linux/fcntl.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/stat.c b/fs/stat.c
index dacecdda2e79..f42d6fa1ec20 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -171,7 +171,7 @@ static int vfs_statx(int dfd, const char __user *filename, int flags,
 	int error;
 
 	if (flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT | AT_EMPTY_PATH |
-		      AT_STATX_SYNC_TYPE))
+		      AT_STATX_SYNC_TYPE | AT_STATX_CACHED))
 		return -EINVAL;
 
 	if (!(flags & AT_SYMLINK_NOFOLLOW))
@@ -180,6 +180,8 @@ static int vfs_statx(int dfd, const char __user *filename, int flags,
 		lookup_flags |= LOOKUP_AUTOMOUNT;
 	if (flags & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
+	if (flags & AT_STATX_CACHED)
+		lookup_flags |= LOOKUP_CACHED;
 
 retry:
 	error = user_path_at(dfd, filename, lookup_flags, &path);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 2f86b2ad6d7e..19d5059393e7 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -111,4 +111,6 @@
 
 #define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
 
+#define AT_STATX_CACHED		0x10000 /* Only succeeds if inode/dentry is already cached */
+
 #endif /* _UAPI_LINUX_FCNTL_H */
-- 
2.30.0

[PATCH 2/3] fs: ensure that ->getattr() honors AT_STATX_CACHED

[Jens Axboe]

For filesystems that provide a private ->getattr() implementation, some of
them need to do IO to satisfy the request. If we need to block off
->getattr() and AT_STATX_CACHED is set, then return -EAGAIN and have the
caller retry.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/9p/vfs_inode.c   | 2 ++
 fs/afs/inode.c      | 3 +++
 fs/ceph/inode.c     | 2 ++
 fs/cifs/inode.c     | 3 +++
 fs/coda/inode.c     | 7 ++++++-
 fs/ecryptfs/inode.c | 3 +++
 fs/fuse/dir.c       | 2 ++
 fs/gfs2/inode.c     | 2 ++
 fs/kernfs/inode.c   | 8 +++++++-
 fs/nfs/inode.c      | 3 +++
 fs/ocfs2/file.c     | 3 +++
 fs/orangefs/inode.c | 3 +++
 fs/ubifs/dir.c      | 7 ++++++-
 fs/udf/symlink.c    | 3 +++
 fs/vboxsf/utils.c   | 4 ++++
 15 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 4a937fac1acb..291d74bcf582 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1030,6 +1030,8 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 		generic_fillattr(d_inode(dentry), stat);
 		return 0;
 	}
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
 	fid = v9fs_fid_lookup(dentry);
 	if (IS_ERR(fid))
 		return PTR_ERR(fid);
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index b0d7b892090d..19ba728ff18f 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -743,6 +743,9 @@ int afs_getattr(const struct path *path, struct kstat *stat,
 
 	_enter("{ ino=%lu v=%u }", inode->i_ino, inode->i_generation);
 
+	if (query_flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	do {
 		read_seqbegin_or_lock(&vnode->cb_lock, &seq);
 		generic_fillattr(inode, stat);
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index adc8fc3c5d85..997f380646fd 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -2378,6 +2378,8 @@ int ceph_getattr(const struct path *path, struct kstat *stat,
 
 	/* Skip the getattr altogether if we're asked not to sync */
 	if (!(flags & AT_STATX_DONT_SYNC)) {
+		if (flags & AT_STATX_CACHED)
+			return -EAGAIN;
 		err = ceph_do_getattr(inode,
 				statx_to_caps(request_mask, inode->i_mode),
 				flags & AT_STATX_FORCE_SYNC);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a83b3a8ffaac..1f8007caa27c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2379,6 +2379,9 @@ int cifs_getattr(const struct path *path, struct kstat *stat,
 	struct inode *inode = d_inode(dentry);
 	int rc;
 
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	/*
 	 * We need to be sure that all dirty pages are written and the server
 	 * has actual ctime, mtime and file length.
diff --git a/fs/coda/inode.c b/fs/coda/inode.c
index b1c70e2b9b1e..444f1ef97b08 100644
--- a/fs/coda/inode.c
+++ b/fs/coda/inode.c
@@ -254,7 +254,12 @@ static void coda_evict_inode(struct inode *inode)
 int coda_getattr(const struct path *path, struct kstat *stat,
 		 u32 request_mask, unsigned int flags)
 {
-	int err = coda_revalidate_inode(d_inode(path->dentry));
+	int err;
+
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
+	err = coda_revalidate_inode(d_inode(path->dentry));
 	if (!err)
 		generic_fillattr(d_inode(path->dentry), stat);
 	return err;
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index e23752d9a79f..61fdb5a0dbdc 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -980,6 +980,9 @@ static int ecryptfs_getattr_link(const struct path *path, struct kstat *stat,
 		char *target;
 		size_t targetsiz;
 
+		if (flags & AT_STATX_CACHED)
+			return -EAGAIN;
+
 		target = ecryptfs_readlink_lower(dentry, &targetsiz);
 		if (!IS_ERR(target)) {
 			kfree(target);
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 78f9f209078c..638722d3c1ed 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1084,6 +1084,8 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 		sync = time_before64(fi->i_time, get_jiffies_64());
 
 	if (sync) {
+		if (flags & AT_STATX_CACHED)
+			return -EAGAIN;
 		forget_all_cached_acls(inode);
 		err = fuse_do_getattr(inode, stat, file);
 	} else if (stat) {
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index c1b77e8d6b1c..3d485d9f1afe 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -2032,6 +2032,8 @@ static int gfs2_getattr(const struct path *path, struct kstat *stat,
 
 	gfs2_holder_mark_uninitialized(&gh);
 	if (gfs2_glock_is_locked_by_me(ip->i_gl) == NULL) {
+		if (flags & AT_STATX_CACHED)
+			return -EAGAIN;
 		error = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, LM_FLAG_ANY, &gh);
 		if (error)
 			return error;
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index fc2469a20fed..2193b3c0b9cd 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -189,7 +189,13 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
 	struct inode *inode = d_inode(path->dentry);
 	struct kernfs_node *kn = inode->i_private;
 
-	mutex_lock(&kernfs_mutex);
+	if (query_flags & AT_STATX_CACHED) {
+		if (!mutex_trylock(&kernfs_mutex))
+			return -EAGAIN;
+	} else {
+		mutex_lock(&kernfs_mutex);
+	}
+
 	kernfs_refresh_inode(kn, inode);
 	mutex_unlock(&kernfs_mutex);
 
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 522aa10a1a3e..1eb167c14884 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -799,6 +799,9 @@ int nfs_getattr(const struct path *path, struct kstat *stat,
 
 	trace_nfs_getattr_enter(inode);
 
+	if (query_flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	if ((query_flags & AT_STATX_DONT_SYNC) && !force_sync) {
 		nfs_readdirplus_parent_cache_hit(path->dentry);
 		goto out_no_update;
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 85979e2214b3..e48d0c33fb46 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1306,6 +1306,9 @@ int ocfs2_getattr(const struct path *path, struct kstat *stat,
 	struct ocfs2_super *osb = sb->s_fs_info;
 	int err;
 
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	err = ocfs2_inode_revalidate(path->dentry);
 	if (err) {
 		if (err != -ENOENT)
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 48f0547d4850..4864334e40e8 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -900,6 +900,9 @@ int orangefs_getattr(const struct path *path, struct kstat *stat,
 		     "orangefs_getattr: called on %pd mask %u\n",
 		     path->dentry, request_mask);
 
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	ret = orangefs_inode_getattr(inode,
 	    request_mask & STATX_SIZE ? ORANGEFS_GETATTR_SIZE : 0);
 	if (ret == 0) {
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 9a6b8660425a..c199b260c50c 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1573,7 +1573,12 @@ int ubifs_getattr(const struct path *path, struct kstat *stat,
 	struct inode *inode = d_inode(path->dentry);
 	struct ubifs_inode *ui = ubifs_inode(inode);
 
-	mutex_lock(&ui->ui_mutex);
+	if (flags & AT_STATX_CACHED) {
+		if (!mutex_trylock(&ui->ui_mutex))
+			return -EAGAIN;
+	} else {
+		mutex_lock(&ui->ui_mutex);
+	}
 
 	if (ui->flags & UBIFS_APPEND_FL)
 		stat->attributes |= STATX_ATTR_APPEND;
diff --git a/fs/udf/symlink.c b/fs/udf/symlink.c
index c973db239604..0edd973b8a43 100644
--- a/fs/udf/symlink.c
+++ b/fs/udf/symlink.c
@@ -159,6 +159,9 @@ static int udf_symlink_getattr(const struct path *path, struct kstat *stat,
 	struct inode *inode = d_backing_inode(dentry);
 	struct page *page;
 
+	if (flags & AT_STATX_CACHED)
+		return -EAGAIN;
+
 	generic_fillattr(inode, stat);
 	page = read_mapping_page(inode->i_mapping, 0, NULL);
 	if (IS_ERR(page))
diff --git a/fs/vboxsf/utils.c b/fs/vboxsf/utils.c
index 018057546067..dc93cd59290d 100644
--- a/fs/vboxsf/utils.c
+++ b/fs/vboxsf/utils.c
@@ -228,6 +228,10 @@ int vboxsf_getattr(const struct path *path, struct kstat *kstat,
 		sf_i->force_restat = 1;
 		fallthrough;
 	default:
+		if (flags & AT_STATX_CACHED) {
+			err = -EAGAIN;
+			break;
+		}
 		err = vboxsf_inode_revalidate(dentry);
 	}
 	if (err)
-- 
2.30.0

[PATCH 3/3] io_uring: use AT_STATX_CACHED for IORING_OP_STATX fast path

[Jens Axboe]

Instead of always going async, we can now attempt a cached attempt by
using AT_STATX_CACHED. This turns into LOOKUP_CACHED, and ensures that
we'll only do a fast path dentry lookup for path resolution.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c246df2f95a4..99799cc5a42e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4394,20 +4394,27 @@ static int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 static int io_statx(struct io_kiocb *req, bool force_nonblock)
 {
 	struct io_statx *ctx = &req->statx;
+	bool cached_set;
 	int ret;
 
-	if (force_nonblock) {
-		/* only need file table for an actual valid fd */
-		if (ctx->dfd == -1 || ctx->dfd == AT_FDCWD)
-			req->flags |= REQ_F_NO_FILE_TABLE;
-		return -EAGAIN;
-	}
+	cached_set = ctx->flags & AT_STATX_CACHED;
+	if (force_nonblock)
+		ctx->flags |= AT_STATX_CACHED;
 
 	ret = do_statx(ctx->dfd, ctx->filename, ctx->flags, ctx->mask,
 		       ctx->buffer);
 
-	if (ret < 0)
+	if (ret < 0) {
+		/* only retry if nonblock wasn't set */
+		if (ret == -EAGAIN && (!cached_set && force_nonblock)) {
+			/* only need file table for an actual valid fd */
+			if (ctx->dfd == -1 || ctx->dfd == AT_FDCWD)
+				req->flags |= REQ_F_NO_FILE_TABLE;
+			ctx->flags &= ~AT_STATX_CACHED;
+			return -EAGAIN;
+		}
 		req_set_fail_links(req);
+	}
 	io_req_complete(req, ret);
 	return 0;
 }
-- 
2.30.0

Re: [PATCHSET RFC] support RESOLVE_CACHED for statx

[Linus Torvalds]

On Mon, Jan 25, 2021 at 1:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>
>     Patch 2 is the
> mostly ugly part, but not sure how we can do this any better - we need
> to ensure that any sort of revalidation or sync in ->getattr() honors
> it too.

Yeah, that's not pretty, but I agree - it looks like this just
requires the filesystem to check whether it needs to revalidate or
not.

But I think that patch could do better than what your patch does. Some
of them are "filesystems could decide to be more finegrained") -  your
cifs patch comes to mind - but some of your "return -EAGAIN if cached"
seem to be just plain pointless.

In afs, for example, you return -EAGAIN instead of just doing the
read-seqlock thing. That's a really cheap CPU-only operation. We're
talking "cheaper than a spinlock" sequence.

           Linus

Re: [PATCHSET RFC] support RESOLVE_CACHED for statx

[Jens Axboe]

On 1/25/21 4:39 PM, Linus Torvalds wrote:
> On Mon, Jan 25, 2021 at 1:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>>     Patch 2 is the
>> mostly ugly part, but not sure how we can do this any better - we need
>> to ensure that any sort of revalidation or sync in ->getattr() honors
>> it too.
> 
> Yeah, that's not pretty, but I agree - it looks like this just
> requires the filesystem to check whether it needs to revalidate or
> not.
> 
> But I think that patch could do better than what your patch does. Some
> of them are "filesystems could decide to be more finegrained") -  your
> cifs patch comes to mind - but some of your "return -EAGAIN if cached"
> seem to be just plain pointless.

Which ones in particular? Outside of the afs one you looked a below,
the rest should all be of the "need to do IO of some sort" and hence
-EAGAIN is reasonable.

cifs could be cleaner, but that'd require more checking in there. I
just tried to keep it simple, and leave the harder work for the
file system developers if they care. If not, it'll still work just
like it does today, we're no worse off there than before (at least
from an io_uring POV).

But I can go ahead and makes eg cifs more accurate in that regard,
if that's what you're objecting to.

> In afs, for example, you return -EAGAIN instead of just doing the
> read-seqlock thing. That's a really cheap CPU-only operation. We're
> talking "cheaper than a spinlock" sequence.

Yep agree on that one, that looks silly and should just go away. I've
killed it.

-- 
Jens Axboe

Re: [PATCHSET RFC] support RESOLVE_CACHED for statx

[Linus Torvalds]

On Mon, Jan 25, 2021 at 5:06 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Which ones in particular? Outside of the afs one you looked a below,
> the rest should all be of the "need to do IO of some sort" and hence
> -EAGAIN is reasonable.

Several of them only do the IO conditionally, which was what I reacted
to in at least cifs.

But I think it's ok to start out doing it unconditionally, and make it
fancier if/when/as people notice or care.

              Linus

Re: [PATCHSET RFC] support RESOLVE_CACHED for statx

[Jens Axboe]

On 1/25/21 6:28 PM, Linus Torvalds wrote:
> On Mon, Jan 25, 2021 at 5:06 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Which ones in particular? Outside of the afs one you looked a below,
>> the rest should all be of the "need to do IO of some sort" and hence
>> -EAGAIN is reasonable.
> 
> Several of them only do the IO conditionally, which was what I reacted
> to in at least cifs.
> 
> But I think it's ok to start out doing it unconditionally, and make it
> fancier if/when/as people notice or care.

Agree, that was the point I was trying to make in the reply. I'd rather
start simpler and talk to folks about improving it, looping in the
relevant developers for that. I'll leave it as-is for now, modulo the
afs one which was definitely just a mistake.

-- 
Jens Axboe