Skip to content

fuse: Allow stale time info for permission getattr #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cding-ddn wants to merge 1 commit into from
Closed

fuse: Allow stale time info for permission getattr #150

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion fs/fuse/dir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,62 @@ static int fuse_do_statx(struct inode *inode, struct file *file,
return 0;
}

/*
* This is used by permission checking to get attributes without requiring
* accurate time information. This allows the FUSE server to acquire a weaker
* lock mode and reduce lock contention.
*/
static int fuse_perm_do_statx(struct inode *inode, struct file *file)
{
int err;
struct fuse_attr attr;
struct fuse_statx *sx;
struct fuse_statx_in inarg;
struct fuse_statx_out outarg;
struct fuse_mount *fm = get_fuse_mount(inode);
u64 attr_version = fuse_get_attr_version(fm->fc);
FUSE_ARGS(args);

memset(&inarg, 0, sizeof(inarg));
memset(&outarg, 0, sizeof(outarg));
/* Directories have separate file-handle space */
if (file && S_ISREG(inode->i_mode)) {
struct fuse_file *ff = file->private_data;

inarg.getattr_flags |= FUSE_GETATTR_FH;
inarg.fh = ff->fh;
}

inarg.sx_flags = 0;
inarg.sx_mask = STATX_BASIC_STATS & ~(STATX_ATIME | STATX_MTIME | STATX_CTIME);
args.opcode = FUSE_STATX;
args.nodeid = get_node_id(inode);
args.in_numargs = 1;
args.in_args[0].size = sizeof(inarg);
args.in_args[0].value = &inarg;
args.out_numargs = 1;
args.out_args[0].size = sizeof(outarg);
args.out_args[0].value = &outarg;
err = fuse_simple_request(fm, &args);
if (err)
return err;

sx = &outarg.stat;
if (((sx->mask & STATX_SIZE) && !fuse_valid_size(sx->size)) ||
((sx->mask & STATX_TYPE) && (!fuse_valid_type(sx->mode) ||
inode_wrong_type(inode, sx->mode)))) {
fuse_make_bad(inode);
return -EIO;
}

fuse_statx_to_attr(&outarg.stat, &attr);
fuse_change_attributes(inode, &attr, &outarg.stat,
ATTR_TIMEOUT(&outarg), attr_version);
fuse_invalidate_attr_mask(inode, STATX_ATIME|STATX_MTIME|STATX_CTIME);

return 0;
}

static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
struct file *file)
{
Comment thread
cding-ddn marked this conversation as resolved.
Expand Down Expand Up @@ -1484,7 +1540,7 @@ static int fuse_perm_getattr(struct inode *inode, int mask)
return -ECHILD;

forget_all_cached_acls(inode);
return fuse_do_getattr(inode, NULL, NULL);
return fuse_perm_do_statx(inode, NULL);
Copy link
Copy Markdown
Collaborator

@bsbernd bsbernd May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that can be much simpler, look at the caller of fuse_perm_getattr, i.e. fuse_permission

u32 perm_mask = STATX_MODE | STATX_UID | STATX_GID;

How about this

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 67581ce277b5..2ae36475e722 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1184,7 +1184,7 @@ static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
 }
 
 static int fuse_do_statx(struct inode *inode, struct file *file,
-                        struct kstat *stat)
+                        struct kstat *stat, int sx_mask)
 {
        int err;
        struct fuse_attr attr;
@@ -1206,7 +1206,7 @@ static int fuse_do_statx(struct inode *inode, struct file *file,
        }
        /* For now leave sync hints as the default, request all stats. */
        inarg.sx_flags = 0;
-       inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
+       inarg.sx_mask = sx_mask;
        args.opcode = FUSE_STATX;
        args.nodeid = get_node_id(inode);
        args.in_numargs = 1;
@@ -1314,9 +1314,8 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 
        if (sync) {
                forget_all_cached_acls(inode);
-               /* Try statx if BTIME is requested */
-               if (!fc->no_statx && (request_mask & ~STATX_BASIC_STATS)) {
-                       err = fuse_do_statx(inode, file, stat);
+               if (!fc->no_statx) {
+                       err = fuse_do_statx(inode, file, stat, request_mask);
                        if (err == -ENOSYS) {
                                fc->no_statx = 1;
                                err = 0;
@@ -1478,13 +1477,13 @@ static int fuse_access(struct inode *inode, int mask)
        return err;
 }
 
-static int fuse_perm_getattr(struct inode *inode, int mask)
+static int fuse_perm_getattr(struct inode *inode, int mask, int perm_mask)
 {
        if (mask & MAY_NOT_BLOCK)
                return -ECHILD;
 
        forget_all_cached_acls(inode);
-       return fuse_do_getattr(inode, NULL, NULL);
+       return fuse_update_get_attr(inode, NULL, NULL, perm_mask, AT_STATX_FORCE_SYNC);
 }
 
 /*
@@ -1506,6 +1505,7 @@ static int fuse_permission(struct mnt_idmap *idmap,
        struct fuse_conn *fc = get_fuse_conn(inode);
        bool refreshed = false;
        int err = 0;
+       int perm_mask = STATX_MODE | STATX_UID | STATX_GID;
 
        if (fuse_is_bad(inode))
                return -EIO;
@@ -1519,13 +1519,12 @@ static int fuse_permission(struct mnt_idmap *idmap,
        if (fc->default_permissions ||
            ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))) {
                struct fuse_inode *fi = get_fuse_inode(inode);
-               u32 perm_mask = STATX_MODE | STATX_UID | STATX_GID;
 
                if (perm_mask & READ_ONCE(fi->inval_mask) ||
                    time_before64(fi->i_time, get_jiffies_64())) {
                        refreshed = true;
 
-                       err = fuse_perm_getattr(inode, mask);
+                       err = fuse_perm_getattr(inode, mask, perm_mask);
                        if (err)
                                return err;
                }
@@ -1538,7 +1537,7 @@ static int fuse_permission(struct mnt_idmap *idmap,
                   attributes.  This is also needed, because the root
                   node will at first have no permissions */
                if (err == -EACCES && !refreshed) {
-                       err = fuse_perm_getattr(inode, mask);
+                       err = fuse_perm_getattr(inode, mask, perm_mask);
                        if (!err)
                                err = generic_permission(&nop_mnt_idmap,
                                                         inode, mask);
@@ -1555,7 +1554,7 @@ static int fuse_permission(struct mnt_idmap *idmap,
                        if (refreshed)
                                return -EACCES;
 
-                       err = fuse_perm_getattr(inode, mask);
+                       err = fuse_perm_getattr(inode, mask, perm_mask);
                        if (!err && !(inode->i_mode & S_IXUGO))
                                return -EACCES;
                }

Sorry, only noticed past midnight when I was just on my way to bed - the above is not even compilation tested.

Basically this passes in the request mask to fuse_statx_to_attr() and uses that function in fuse_perm_getattr() via fuse_update_get_attr().
I don't know if Miklos will accept it like this, but I don't think he can reject it entirely either, because as I said before, one of the reasons to create statx was the network file system case with a request mask - the file system is obliged to fill in the requested arguments, but is free to fill in more.

Copy link
Copy Markdown
Collaborator

@bsbernd bsbernd May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With "much simpler" I actually mean "without code dup".

Copy link
Copy Markdown
Collaborator Author

@cding-ddn cding-ddn May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that may cause problem when we cherry-pick upstream kernel changes later.

Copy link
Copy Markdown
Collaborator Author

@cding-ddn cding-ddn May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean my change intentionally does not modify the existing kernel routine signature.

Copy link
Copy Markdown
Collaborator

@bsbernd bsbernd May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously we need to send my suggestion upstream.

Copy link
Copy Markdown
Collaborator Author

@cding-ddn cding-ddn May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsbernd The suggested code fails to improve performance; fuse_do_statx() does not invoke redfs_change_attributes() for the requested mask STATX_MODE | STATX_UID | STATX_GID.

}

/*
Expand Down