[PATCH 0/5] fuse: support birth time

[PATCH 0/5] fuse: support birth time

[Miklos Szeredi]

Add the infrastructure for btime support in the form of a new STATX
request.

The format of the STATX reply is binary compatible with struct statx on
Linux, but the structure is defined separately in the fuse API for other
OS's.

Currently STATX only supports basic attributes, same as the GETATTR
request, plus btime.  But nothing prevents extending support to other
(present and future) statx fields.

---
Miklos Szeredi (5):
  fuse: handle empty request_mask in statx
  fuse: add STATX request
  fuse: add ATTR_TIMEOUT macro
  fuse: implement statx
  fuse: cache btime

 fs/fuse/dir.c             | 145 +++++++++++++++++++++++++++++++-------
 fs/fuse/fuse_i.h          |  15 +++-
 fs/fuse/inode.c           |  30 ++++++--
 fs/fuse/readdir.c         |   6 +-
 include/uapi/linux/fuse.h |  56 ++++++++++++++-
 5 files changed, 218 insertions(+), 34 deletions(-)

-- 
2.40.1

[PATCH 1/5] fuse: handle empty request_mask in statx

[Miklos Szeredi]

If no attribute is requested, then don't send request to userspace.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index f67bef9d83c4..d38ab93e2007 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1209,7 +1209,12 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 	u32 inval_mask = READ_ONCE(fi->inval_mask);
 	u32 cache_mask = fuse_get_cache_mask(inode);
 
-	if (flags & AT_STATX_FORCE_SYNC)
+	/* FUSE only supports basic stats */
+	request_mask &= STATX_BASIC_STATS;
+
+	if (!request_mask)
+		sync = false;
+	else if (flags & AT_STATX_FORCE_SYNC)
 		sync = true;
 	else if (flags & AT_STATX_DONT_SYNC)
 		sync = false;
-- 
2.40.1

[PATCH 2/5] fuse: add STATX request

[Miklos Szeredi]

Use the same structure as statx.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 include/uapi/linux/fuse.h | 56 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index b3fcab13fcd3..fe700b91b33b 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -207,6 +207,9 @@
  *  - add FUSE_EXT_GROUPS
  *  - add FUSE_CREATE_SUPP_GROUP
  *  - add FUSE_HAS_EXPIRE_ONLY
+ *
+ *  7.39
+ *  - add FUSE_STATX and related structures
  */
 
 #ifndef _LINUX_FUSE_H
@@ -242,7 +245,7 @@
 #define FUSE_KERNEL_VERSION 7
 
 /** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 38
+#define FUSE_KERNEL_MINOR_VERSION 39
 
 /** The node ID of the root inode */
 #define FUSE_ROOT_ID 1
@@ -269,6 +272,40 @@ struct fuse_attr {
 	uint32_t	flags;
 };
 
+/*
+ * The following structures are bit-for-bit compatible with the statx(2) ABI in
+ * Linux.
+ */
+struct fuse_sx_time {
+	int64_t		tv_sec;
+	uint32_t	tv_nsec;
+	int32_t		__reserved;
+};
+
+struct fuse_statx {
+	uint32_t	mask;
+	uint32_t	blksize;
+	uint64_t	attributes;
+	uint32_t	nlink;
+	uint32_t	uid;
+	uint32_t	gid;
+	uint16_t	mode;
+	uint16_t	__spare0[1];
+	uint64_t	ino;
+	uint64_t	size;
+	uint64_t	blocks;
+	uint64_t	attributes_mask;
+	struct fuse_sx_time	atime;
+	struct fuse_sx_time	btime;
+	struct fuse_sx_time	ctime;
+	struct fuse_sx_time	mtime;
+	uint32_t	rdev_major;
+	uint32_t	rdev_minor;
+	uint32_t	dev_major;
+	uint32_t	dev_minor;
+	uint64_t	__spare2[14];
+};
+
 struct fuse_kstatfs {
 	uint64_t	blocks;
 	uint64_t	bfree;
@@ -575,6 +612,7 @@ enum fuse_opcode {
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
 	FUSE_TMPFILE		= 51,
+	FUSE_STATX		= 52,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
@@ -639,6 +677,22 @@ struct fuse_attr_out {
 	struct fuse_attr attr;
 };
 
+struct fuse_statx_in {
+	uint32_t	getattr_flags;
+	uint32_t	reserved;
+	uint64_t	fh;
+	uint32_t	sx_flags;
+	uint32_t	sx_mask;
+};
+
+struct fuse_statx_out {
+	uint64_t	attr_valid;	/* Cache timeout for the attributes */
+	uint32_t	attr_valid_nsec;
+	uint32_t	flags;
+	uint64_t	spare[2];
+	struct fuse_statx stat;
+};
+
 #define FUSE_COMPAT_MKNOD_IN_SIZE 8
 
 struct fuse_mknod_in {
-- 
2.40.1

[PATCH 3/5] fuse: add ATTR_TIMEOUT macro

[Miklos Szeredi]

Next patch will introduce yet another type attribute reply.  Add a macro
that can handle attribute timeouts for all of the structs.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c     | 26 ++++++++------------------
 fs/fuse/fuse_i.h  |  5 ++++-
 fs/fuse/readdir.c |  4 ++--
 3 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d38ab93e2007..04006db6e173 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -92,7 +92,7 @@ static void fuse_dentry_settime(struct dentry *dentry, u64 time)
 /*
  * Calculate the time in jiffies until a dentry/attributes are valid
  */
-static u64 time_to_jiffies(u64 sec, u32 nsec)
+u64 fuse_time_to_jiffies(u64 sec, u32 nsec)
 {
 	if (sec || nsec) {
 		struct timespec64 ts = {
@@ -112,17 +112,7 @@ static u64 time_to_jiffies(u64 sec, u32 nsec)
 void fuse_change_entry_timeout(struct dentry *entry, struct fuse_entry_out *o)
 {
 	fuse_dentry_settime(entry,
-		time_to_jiffies(o->entry_valid, o->entry_valid_nsec));
-}
-
-static u64 attr_timeout(struct fuse_attr_out *o)
-{
-	return time_to_jiffies(o->attr_valid, o->attr_valid_nsec);
-}
-
-u64 entry_attr_timeout(struct fuse_entry_out *o)
-{
-	return time_to_jiffies(o->attr_valid, o->attr_valid_nsec);
+		fuse_time_to_jiffies(o->entry_valid, o->entry_valid_nsec));
 }
 
 void fuse_invalidate_attr_mask(struct inode *inode, u32 mask)
@@ -266,7 +256,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 
 		forget_all_cached_acls(inode);
 		fuse_change_attributes(inode, &outarg.attr,
-				       entry_attr_timeout(&outarg),
+				       ATTR_TIMEOUT(&outarg),
 				       attr_version);
 		fuse_change_entry_timeout(entry, &outarg);
 	} else if (inode) {
@@ -399,7 +389,7 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 		goto out_put_forget;
 
 	*inode = fuse_iget(sb, outarg->nodeid, outarg->generation,
-			   &outarg->attr, entry_attr_timeout(outarg),
+			   &outarg->attr, ATTR_TIMEOUT(outarg),
 			   attr_version);
 	err = -ENOMEM;
 	if (!*inode) {
@@ -686,7 +676,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	ff->nodeid = outentry.nodeid;
 	ff->open_flags = outopen.open_flags;
 	inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
-			  &outentry.attr, entry_attr_timeout(&outentry), 0);
+			  &outentry.attr, ATTR_TIMEOUT(&outentry), 0);
 	if (!inode) {
 		flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
 		fuse_sync_release(NULL, ff, flags);
@@ -813,7 +803,7 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 		goto out_put_forget_req;
 
 	inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
-			  &outarg.attr, entry_attr_timeout(&outarg), 0);
+			  &outarg.attr, ATTR_TIMEOUT(&outarg), 0);
 	if (!inode) {
 		fuse_queue_forget(fm->fc, forget, outarg.nodeid, 1);
 		return -ENOMEM;
@@ -1190,7 +1180,7 @@ static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
 			err = -EIO;
 		} else {
 			fuse_change_attributes(inode, &outarg.attr,
-					       attr_timeout(&outarg),
+					       ATTR_TIMEOUT(&outarg),
 					       attr_version);
 			if (stat)
 				fuse_fillattr(inode, &outarg.attr, stat);
@@ -1867,7 +1857,7 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 	}
 
 	fuse_change_attributes_common(inode, &outarg.attr,
-				      attr_timeout(&outarg),
+				      ATTR_TIMEOUT(&outarg),
 				      fuse_get_cache_mask(inode));
 	oldsize = inode->i_size;
 	/* see the comment in fuse_change_attributes() */
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 9b7fc7d3c7f1..fd55c09514cd 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1111,7 +1111,10 @@ void fuse_invalidate_entry_cache(struct dentry *entry);
 
 void fuse_invalidate_atime(struct inode *inode);
 
-u64 entry_attr_timeout(struct fuse_entry_out *o);
+u64 fuse_time_to_jiffies(u64 sec, u32 nsec);
+#define ATTR_TIMEOUT(o) \
+	fuse_time_to_jiffies((o)->attr_valid, (o)->attr_valid_nsec)
+
 void fuse_change_entry_timeout(struct dentry *entry, struct fuse_entry_out *o);
 
 /**
diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
index dc603479b30e..48b3a6ec278b 100644
--- a/fs/fuse/readdir.c
+++ b/fs/fuse/readdir.c
@@ -224,7 +224,7 @@ static int fuse_direntplus_link(struct file *file,
 
 		forget_all_cached_acls(inode);
 		fuse_change_attributes(inode, &o->attr,
-				       entry_attr_timeout(o),
+				       ATTR_TIMEOUT(o),
 				       attr_version);
 		/*
 		 * The other branch comes via fuse_iget()
@@ -232,7 +232,7 @@ static int fuse_direntplus_link(struct file *file,
 		 */
 	} else {
 		inode = fuse_iget(dir->i_sb, o->nodeid, o->generation,
-				  &o->attr, entry_attr_timeout(o),
+				  &o->attr, ATTR_TIMEOUT(o),
 				  attr_version);
 		if (!inode)
 			inode = ERR_PTR(-ENOMEM);
-- 
2.40.1

[PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

Allow querying btime.  When btime is requested in mask, then FUSE_STATX
request is sent.  Otherwise keep using FUSE_GETATTR.

The userspace interface for statx matches that of the statx(2) API.
However there are limitations on how this interface is used:

 - returned basic stats and btime are used, stx_attributes, etc. are
   ignored

 - always query basic stats and btime, regardless of what was requested

 - requested sync type is ignored, the default is passed to the server

 - if server returns with some attributes missing from the result_mask,
   then no attributes will be cached

 - btime is not cached yet (next patch will fix that)

For new inodes initialize fi->inval_mask to "all invalid", instead of "all
valid" as previously.  Also only clear basic stats from inval_mask when
caching attributes.  This will result in the caching logic not thinking
that btime is cached.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++---
 fs/fuse/fuse_i.h |   3 ++
 fs/fuse/inode.c  |   5 ++-
 3 files changed, 107 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 04006db6e173..552157bd6a4d 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -350,10 +350,14 @@ int fuse_valid_type(int m)
 		S_ISBLK(m) || S_ISFIFO(m) || S_ISSOCK(m);
 }
 
+bool fuse_valid_size(u64 size)
+{
+	return size <= LLONG_MAX;
+}
+
 bool fuse_invalid_attr(struct fuse_attr *attr)
 {
-	return !fuse_valid_type(attr->mode) ||
-		attr->size > LLONG_MAX;
+	return !fuse_valid_type(attr->mode) || !fuse_valid_size(attr->size);
 }
 
 int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name,
@@ -1143,6 +1147,84 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
 	stat->blksize = 1 << blkbits;
 }
 
+static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
+{
+	memset(attr, 0, sizeof(*attr));
+	attr->ino = sx->ino;
+	attr->size = sx->size;
+	attr->blocks = sx->blocks;
+	attr->atime = sx->atime.tv_sec;
+	attr->mtime = sx->mtime.tv_sec;
+	attr->ctime = sx->ctime.tv_sec;
+	attr->atimensec = sx->atime.tv_nsec;
+	attr->mtimensec = sx->mtime.tv_nsec;
+	attr->ctimensec = sx->ctime.tv_nsec;
+	attr->mode = sx->mode;
+	attr->nlink = sx->nlink;
+	attr->uid = sx->uid;
+	attr->gid = sx->gid;
+	attr->rdev = new_encode_dev(MKDEV(sx->rdev_major, sx->rdev_minor));
+	attr->blksize = sx->blksize;
+}
+
+static int fuse_do_statx(struct inode *inode, struct file *file,
+			 struct kstat *stat)
+{
+	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;
+	}
+	/* For now leave sync hints as the default, request all stats. */
+	inarg.sx_flags = 0;
+	inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
+	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)))) {
+		make_bad_inode(inode);
+		return -EIO;
+	}
+
+	fuse_statx_to_attr(&outarg.stat, &attr);
+	if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
+		fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
+				       attr_version);
+	}
+	stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
+	stat->btime.tv_sec = sx->btime.tv_sec;
+	stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
+	fuse_fillattr(inode, &attr, stat);
+	stat->result_mask |= STATX_TYPE;
+
+	return 0;
+}
+
 static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
 			   struct file *file)
 {
@@ -1194,13 +1276,18 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 				unsigned int flags)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
+	struct fuse_conn *fc = get_fuse_conn(inode);
 	int err = 0;
 	bool sync;
 	u32 inval_mask = READ_ONCE(fi->inval_mask);
 	u32 cache_mask = fuse_get_cache_mask(inode);
 
-	/* FUSE only supports basic stats */
-	request_mask &= STATX_BASIC_STATS;
+
+	/* FUSE only supports basic stats and possibly btime */
+	request_mask &= STATX_BASIC_STATS | STATX_BTIME;
+retry:
+	if (fc->no_statx)
+		request_mask &= STATX_BASIC_STATS;
 
 	if (!request_mask)
 		sync = false;
@@ -1215,7 +1302,16 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 
 	if (sync) {
 		forget_all_cached_acls(inode);
-		err = fuse_do_getattr(inode, stat, file);
+		/* Try statx if BTIME is requested */
+		if (!fc->no_statx && (request_mask & ~STATX_BASIC_STATS)) {
+			err = fuse_do_statx(inode, file, stat);
+			if (err == -ENOSYS) {
+				fc->no_statx = 1;
+				goto retry;
+			}
+		} else {
+			err = fuse_do_getattr(inode, stat, file);
+		}
 	} else if (stat) {
 		generic_fillattr(&nop_mnt_idmap, inode, stat);
 		stat->mode = fi->orig_i_mode;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index fd55c09514cd..daae31c58754 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -792,6 +792,9 @@ struct fuse_conn {
 	/* Is tmpfile not implemented by fs? */
 	unsigned int no_tmpfile:1;
 
+	/* Is statx not implemented by fs? */
+	unsigned int no_statx:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index f19d748890f0..a6cc102e66bc 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -77,7 +77,7 @@ static struct inode *fuse_alloc_inode(struct super_block *sb)
 		return NULL;
 
 	fi->i_time = 0;
-	fi->inval_mask = 0;
+	fi->inval_mask = ~0;
 	fi->nodeid = 0;
 	fi->nlookup = 0;
 	fi->attr_version = 0;
@@ -172,7 +172,8 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 
 	fi->attr_version = atomic64_inc_return(&fc->attr_version);
 	fi->i_time = attr_valid;
-	WRITE_ONCE(fi->inval_mask, 0);
+	/* Clear basic stats from invalid mask */
+	set_mask_bits(&fi->inval_mask, STATX_BASIC_STATS, 0);
 
 	inode->i_ino     = fuse_squash_ino(attr->ino);
 	inode->i_mode    = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
-- 
2.40.1

[PATCH 5/5] fuse: cache btime

[Miklos Szeredi]

Not all inode attributes are supported by all filesystems, but for the
basic stats (which are returned by stat(2) and friends) all of them will
have some value, even if that doesn't reflect a real attribute of the file.

Btime is different, in that filesystems are free to report or not report a
value in statx.  If the value is available, then STATX_BTIME bit is set in
stx_mask.

When caching the value of btime, remember the availability of the attribute
as well as the value (if available).  This is done by using the
FUSE_I_BTIME bit in fuse_inode->state to indicate availability, while using
fuse_inode->inval_mask & STATX_BTIME to indicate the state of the cache
itself (i.e. set if cache is invalid, and cleared if cache is valid).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c     | 14 +++++++++-----
 fs/fuse/fuse_i.h  |  7 +++++++
 fs/fuse/inode.c   | 25 +++++++++++++++++++++++--
 fs/fuse/readdir.c |  2 +-
 4 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 552157bd6a4d..42f49fe6e770 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -255,7 +255,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 			goto invalid;
 
 		forget_all_cached_acls(inode);
-		fuse_change_attributes(inode, &outarg.attr,
+		fuse_change_attributes(inode, &outarg.attr, NULL,
 				       ATTR_TIMEOUT(&outarg),
 				       attr_version);
 		fuse_change_entry_timeout(entry, &outarg);
@@ -1213,8 +1213,8 @@ static int fuse_do_statx(struct inode *inode, struct file *file,
 
 	fuse_statx_to_attr(&outarg.stat, &attr);
 	if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
-		fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
-				       attr_version);
+		fuse_change_attributes(inode, &attr, &outarg.stat,
+				       ATTR_TIMEOUT(&outarg), attr_version);
 	}
 	stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
 	stat->btime.tv_sec = sx->btime.tv_sec;
@@ -1261,7 +1261,7 @@ static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
 			fuse_make_bad(inode);
 			err = -EIO;
 		} else {
-			fuse_change_attributes(inode, &outarg.attr,
+			fuse_change_attributes(inode, &outarg.attr, NULL,
 					       ATTR_TIMEOUT(&outarg),
 					       attr_version);
 			if (stat)
@@ -1316,6 +1316,10 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
 		generic_fillattr(&nop_mnt_idmap, inode, stat);
 		stat->mode = fi->orig_i_mode;
 		stat->ino = fi->orig_ino;
+		if (test_bit(FUSE_I_BTIME, &fi->state)) {
+			stat->btime = fi->i_btime;
+			stat->result_mask |= STATX_BTIME;
+		}
 	}
 
 	return err;
@@ -1952,7 +1956,7 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr,
 		/* FIXME: clear I_DIRTY_SYNC? */
 	}
 
-	fuse_change_attributes_common(inode, &outarg.attr,
+	fuse_change_attributes_common(inode, &outarg.attr, NULL,
 				      ATTR_TIMEOUT(&outarg),
 				      fuse_get_cache_mask(inode));
 	oldsize = inode->i_size;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index daae31c58754..4608c3deab52 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -88,6 +88,9 @@ struct fuse_inode {
 	    preserve the original mode */
 	umode_t orig_i_mode;
 
+	/* Cache birthtime */
+	struct timespec64 i_btime;
+
 	/** 64 bit inode number */
 	u64 orig_ino;
 
@@ -167,6 +170,8 @@ enum {
 	FUSE_I_SIZE_UNSTABLE,
 	/* Bad inode */
 	FUSE_I_BAD,
+	/* Has btime */
+	FUSE_I_BTIME,
 };
 
 struct fuse_conn;
@@ -1061,9 +1066,11 @@ void fuse_init_symlink(struct inode *inode);
  * Change attributes of an inode
  */
 void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
+			    struct fuse_statx *sx,
 			    u64 attr_valid, u64 attr_version);
 
 void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
+				   struct fuse_statx *sx,
 				   u64 attr_valid, u32 cache_mask);
 
 u32 fuse_get_cache_mask(struct inode *inode);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index a6cc102e66bc..175ac7e4e06d 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -163,6 +163,7 @@ static ino_t fuse_squash_ino(u64 ino64)
 }
 
 void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
+				   struct fuse_statx *sx,
 				   u64 attr_valid, u32 cache_mask)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
@@ -198,6 +199,25 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 		inode->i_ctime.tv_sec   = attr->ctime;
 		inode->i_ctime.tv_nsec  = attr->ctimensec;
 	}
+	if (sx) {
+		/* Sanitize nsecs */
+		sx->btime.tv_nsec =
+			min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
+
+		/*
+		 * Btime has been queried, cache is valid (whether or not btime
+		 * is available or not) so clear STATX_BTIME from inval_mask.
+		 *
+		 * Availability of the btime attribute is indicated in
+		 * FUSE_I_BTIME
+		 */
+		set_mask_bits(&fi->inval_mask, STATX_BTIME, 0);
+		if (sx->mask & STATX_BTIME) {
+			set_bit(FUSE_I_BTIME, &fi->state);
+			fi->i_btime.tv_sec = sx->btime.tv_sec;
+			fi->i_btime.tv_nsec = sx->btime.tv_nsec;
+		}
+	}
 
 	if (attr->blksize != 0)
 		inode->i_blkbits = ilog2(attr->blksize);
@@ -237,6 +257,7 @@ u32 fuse_get_cache_mask(struct inode *inode)
 }
 
 void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
+			    struct fuse_statx *sx,
 			    u64 attr_valid, u64 attr_version)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
@@ -271,7 +292,7 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
 	}
 
 	old_mtime = inode->i_mtime;
-	fuse_change_attributes_common(inode, attr, attr_valid, cache_mask);
+	fuse_change_attributes_common(inode, attr, sx, attr_valid, cache_mask);
 
 	oldsize = inode->i_size;
 	/*
@@ -409,7 +430,7 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 	spin_lock(&fi->lock);
 	fi->nlookup++;
 	spin_unlock(&fi->lock);
-	fuse_change_attributes(inode, attr, attr_valid, attr_version);
+	fuse_change_attributes(inode, attr, NULL, attr_valid, attr_version);
 
 	return inode;
 }
diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c
index 48b3a6ec278b..1c5e5bfb5d58 100644
--- a/fs/fuse/readdir.c
+++ b/fs/fuse/readdir.c
@@ -223,7 +223,7 @@ static int fuse_direntplus_link(struct file *file,
 		spin_unlock(&fi->lock);
 
 		forget_all_cached_acls(inode);
-		fuse_change_attributes(inode, &o->attr,
+		fuse_change_attributes(inode, &o->attr, NULL,
 				       ATTR_TIMEOUT(o),
 				       attr_version);
 		/*
-- 
2.40.1

Re: [PATCH 2/5] fuse: add STATX request

[Bernd Schubert]

On 8/10/23 12:54, Miklos Szeredi wrote:
> Use the same structure as statx.

Wouldn't it be easier to just include struct statx? Or is there an issue 
with __u32, etc? If so, just a sufficiently large array could be used 
and statx values just mem-copied in/out?

> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>   include/uapi/linux/fuse.h | 56 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index b3fcab13fcd3..fe700b91b33b 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -207,6 +207,9 @@
>    *  - add FUSE_EXT_GROUPS
>    *  - add FUSE_CREATE_SUPP_GROUP
>    *  - add FUSE_HAS_EXPIRE_ONLY
> + *
> + *  7.39
> + *  - add FUSE_STATX and related structures
>    */
>   
>   #ifndef _LINUX_FUSE_H
> @@ -242,7 +245,7 @@
>   #define FUSE_KERNEL_VERSION 7
>   
>   /** Minor version number of this interface */
> -#define FUSE_KERNEL_MINOR_VERSION 38
> +#define FUSE_KERNEL_MINOR_VERSION 39
>   
>   /** The node ID of the root inode */
>   #define FUSE_ROOT_ID 1
> @@ -269,6 +272,40 @@ struct fuse_attr {
>   	uint32_t	flags;
>   };
>   
> +/*
> + * The following structures are bit-for-bit compatible with the statx(2) ABI in
> + * Linux.
> + */
> +struct fuse_sx_time {
> +	int64_t		tv_sec;
> +	uint32_t	tv_nsec;
> +	int32_t		__reserved;
> +};
> +
> +struct fuse_statx {
> +	uint32_t	mask;
> +	uint32_t	blksize;
> +	uint64_t	attributes;
> +	uint32_t	nlink;
> +	uint32_t	uid;
> +	uint32_t	gid;
> +	uint16_t	mode;
> +	uint16_t	__spare0[1];
> +	uint64_t	ino;
> +	uint64_t	size;
> +	uint64_t	blocks;
> +	uint64_t	attributes_mask;
> +	struct fuse_sx_time	atime;
> +	struct fuse_sx_time	btime;
> +	struct fuse_sx_time	ctime;
> +	struct fuse_sx_time	mtime;
> +	uint32_t	rdev_major;
> +	uint32_t	rdev_minor;
> +	uint32_t	dev_major;
> +	uint32_t	dev_minor;
> +	uint64_t	__spare2[14];
> +};

Looks like some recent values are missing?

	/* 0x90 */
	__u64	stx_mnt_id;
	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
	/* 0xa0 */
	__u64	__spare3[12];	/* Spare space for future expansion */
	/* 0x100 */

Which is basically why my personal preference would be not to do have a 
copy of the struct - there is maintenance overhead.


> +
>   struct fuse_kstatfs {
>   	uint64_t	blocks;
>   	uint64_t	bfree;
> @@ -575,6 +612,7 @@ enum fuse_opcode {
>   	FUSE_REMOVEMAPPING	= 49,
>   	FUSE_SYNCFS		= 50,
>   	FUSE_TMPFILE		= 51,
> +	FUSE_STATX		= 52,
>   
>   	/* CUSE specific operations */
>   	CUSE_INIT		= 4096,
> @@ -639,6 +677,22 @@ struct fuse_attr_out {
>   	struct fuse_attr attr;
>   };
>   
> +struct fuse_statx_in {
> +	uint32_t	getattr_flags;
> +	uint32_t	reserved;
> +	uint64_t	fh;
> +	uint32_t	sx_flags;
> +	uint32_t	sx_mask;
> +};
> +
> +struct fuse_statx_out {
> +	uint64_t	attr_valid;	/* Cache timeout for the attributes */
> +	uint32_t	attr_valid_nsec;
> +	uint32_t	flags;
> +	uint64_t	spare[2];
> +	struct fuse_statx stat;
> +};
> +
>   #define FUSE_COMPAT_MKNOD_IN_SIZE 8
>   
>   struct fuse_mknod_in {


Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[kernel test robot]

Hi Miklos,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mszeredi-fuse/for-next]
[also build test WARNING on linus/master v6.5-rc5 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Miklos-Szeredi/fuse-handle-empty-request_mask-in-statx/20230810-185708
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git for-next
patch link:    https://lore.kernel.org/r/20230810105501.1418427-5-mszeredi%40redhat.com
patch subject: [PATCH 4/5] fuse: implement statx
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230810/202308102130.EEqF5GG3-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230810/202308102130.EEqF5GG3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308102130.EEqF5GG3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/fuse/dir.c:353:6: warning: no previous prototype for 'fuse_valid_size' [-Wmissing-prototypes]
     353 | bool fuse_valid_size(u64 size)
         |      ^~~~~~~~~~~~~~~


vim +/fuse_valid_size +353 fs/fuse/dir.c

   352	
 > 353	bool fuse_valid_size(u64 size)
   354	{
   355		return size <= LLONG_MAX;
   356	}
   357	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/5] fuse: add STATX request

[Miklos Szeredi]

On Thu, 10 Aug 2023 at 15:23, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/10/23 12:54, Miklos Szeredi wrote:
> > Use the same structure as statx.
>
> Wouldn't it be easier to just include struct statx? Or is there an issue
> with __u32, etc? If so, just a sufficiently large array could be used
> and statx values just mem-copied in/out?

<linux/uapi/fuse.h> is OS independent.  Ports can grab it and use it
in their userspace and kernel implementations.


>
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >   include/uapi/linux/fuse.h | 56 ++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index b3fcab13fcd3..fe700b91b33b 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -207,6 +207,9 @@
> >    *  - add FUSE_EXT_GROUPS
> >    *  - add FUSE_CREATE_SUPP_GROUP
> >    *  - add FUSE_HAS_EXPIRE_ONLY
> > + *
> > + *  7.39
> > + *  - add FUSE_STATX and related structures
> >    */
> >
> >   #ifndef _LINUX_FUSE_H
> > @@ -242,7 +245,7 @@
> >   #define FUSE_KERNEL_VERSION 7
> >
> >   /** Minor version number of this interface */
> > -#define FUSE_KERNEL_MINOR_VERSION 38
> > +#define FUSE_KERNEL_MINOR_VERSION 39
> >
> >   /** The node ID of the root inode */
> >   #define FUSE_ROOT_ID 1
> > @@ -269,6 +272,40 @@ struct fuse_attr {
> >       uint32_t        flags;
> >   };
> >
> > +/*
> > + * The following structures are bit-for-bit compatible with the statx(2) ABI in
> > + * Linux.
> > + */
> > +struct fuse_sx_time {
> > +     int64_t         tv_sec;
> > +     uint32_t        tv_nsec;
> > +     int32_t         __reserved;
> > +};
> > +
> > +struct fuse_statx {
> > +     uint32_t        mask;
> > +     uint32_t        blksize;
> > +     uint64_t        attributes;
> > +     uint32_t        nlink;
> > +     uint32_t        uid;
> > +     uint32_t        gid;
> > +     uint16_t        mode;
> > +     uint16_t        __spare0[1];
> > +     uint64_t        ino;
> > +     uint64_t        size;
> > +     uint64_t        blocks;
> > +     uint64_t        attributes_mask;
> > +     struct fuse_sx_time     atime;
> > +     struct fuse_sx_time     btime;
> > +     struct fuse_sx_time     ctime;
> > +     struct fuse_sx_time     mtime;
> > +     uint32_t        rdev_major;
> > +     uint32_t        rdev_minor;
> > +     uint32_t        dev_major;
> > +     uint32_t        dev_minor;
> > +     uint64_t        __spare2[14];
> > +};
>
> Looks like some recent values are missing?

It doesn't matter, since those parts are not used.

>
>         /* 0x90 */
>         __u64   stx_mnt_id;
>         __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
>         __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
>         /* 0xa0 */
>         __u64   __spare3[12];   /* Spare space for future expansion */
>         /* 0x100 */
>
> Which is basically why my personal preference would be not to do have a
> copy of the struct - there is maintenance overhead.

Whenever the new fields would be used in the kernel the fields can be
added.  So no need to continually update the one in fuse, since those
fields cannot be referenced by the kernel.   Userspace might actually
access those fields through struct statx, but that's okay, the
interface was designed so that the producer and consumer can use
different versions of the API.

Thanks,
Miklos

Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

On Thu, Aug 10, 2023 at 09:34:59PM +0800, kernel test robot wrote:
> Hi Miklos,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on mszeredi-fuse/for-next]


Thanks.

Updated patch:

---
From: Miklos Szeredi <mszeredi@redhat.com>
Subject: fuse: implement statx

Allow querying btime.  When btime is requested in mask, then FUSE_STATX
request is sent.  Otherwise keep using FUSE_GETATTR.

The userspace interface for statx matches that of the statx(2) API.
However there are limitations on how this interface is used:

 - returned basic stats and btime are used, stx_attributes, etc. are
   ignored

 - always query basic stats and btime, regardless of what was requested

 - requested sync type is ignored, the default is passed to the server

 - if server returns with some attributes missing from the result_mask,
   then no attributes will be cached

 - btime is not cached yet (next patch will fix that)

For new inodes initialize fi->inval_mask to "all invalid", instead of "all
valid" as previously.  Also only clear basic stats from inval_mask when
caching attributes.  This will result in the caching logic not thinking
that btime is cached.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c    |  106 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 fs/fuse/fuse_i.h |    3 +
 fs/fuse/inode.c  |    5 +-
 3 files changed, 107 insertions(+), 7 deletions(-)

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -350,10 +350,14 @@ int fuse_valid_type(int m)
 		S_ISBLK(m) || S_ISFIFO(m) || S_ISSOCK(m);
 }
 
+static bool fuse_valid_size(u64 size)
+{
+	return size <= LLONG_MAX;
+}
+
 bool fuse_invalid_attr(struct fuse_attr *attr)
 {
-	return !fuse_valid_type(attr->mode) ||
-		attr->size > LLONG_MAX;
+	return !fuse_valid_type(attr->mode) || !fuse_valid_size(attr->size);
 }
 
 int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name,
@@ -1143,6 +1147,84 @@ static void fuse_fillattr(struct inode *
 	stat->blksize = 1 << blkbits;
 }
 
+static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
+{
+	memset(attr, 0, sizeof(*attr));
+	attr->ino = sx->ino;
+	attr->size = sx->size;
+	attr->blocks = sx->blocks;
+	attr->atime = sx->atime.tv_sec;
+	attr->mtime = sx->mtime.tv_sec;
+	attr->ctime = sx->ctime.tv_sec;
+	attr->atimensec = sx->atime.tv_nsec;
+	attr->mtimensec = sx->mtime.tv_nsec;
+	attr->ctimensec = sx->ctime.tv_nsec;
+	attr->mode = sx->mode;
+	attr->nlink = sx->nlink;
+	attr->uid = sx->uid;
+	attr->gid = sx->gid;
+	attr->rdev = new_encode_dev(MKDEV(sx->rdev_major, sx->rdev_minor));
+	attr->blksize = sx->blksize;
+}
+
+static int fuse_do_statx(struct inode *inode, struct file *file,
+			 struct kstat *stat)
+{
+	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;
+	}
+	/* For now leave sync hints as the default, request all stats. */
+	inarg.sx_flags = 0;
+	inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
+	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)))) {
+		make_bad_inode(inode);
+		return -EIO;
+	}
+
+	fuse_statx_to_attr(&outarg.stat, &attr);
+	if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
+		fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
+				       attr_version);
+	}
+	stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
+	stat->btime.tv_sec = sx->btime.tv_sec;
+	stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
+	fuse_fillattr(inode, &attr, stat);
+	stat->result_mask |= STATX_TYPE;
+
+	return 0;
+}
+
 static int fuse_do_getattr(struct inode *inode, struct kstat *stat,
 			   struct file *file)
 {
@@ -1194,13 +1276,18 @@ static int fuse_update_get_attr(struct i
 				unsigned int flags)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
+	struct fuse_conn *fc = get_fuse_conn(inode);
 	int err = 0;
 	bool sync;
 	u32 inval_mask = READ_ONCE(fi->inval_mask);
 	u32 cache_mask = fuse_get_cache_mask(inode);
 
-	/* FUSE only supports basic stats */
-	request_mask &= STATX_BASIC_STATS;
+
+	/* FUSE only supports basic stats and possibly btime */
+	request_mask &= STATX_BASIC_STATS | STATX_BTIME;
+retry:
+	if (fc->no_statx)
+		request_mask &= STATX_BASIC_STATS;
 
 	if (!request_mask)
 		sync = false;
@@ -1215,7 +1302,16 @@ static int fuse_update_get_attr(struct i
 
 	if (sync) {
 		forget_all_cached_acls(inode);
-		err = fuse_do_getattr(inode, stat, file);
+		/* Try statx if BTIME is requested */
+		if (!fc->no_statx && (request_mask & ~STATX_BASIC_STATS)) {
+			err = fuse_do_statx(inode, file, stat);
+			if (err == -ENOSYS) {
+				fc->no_statx = 1;
+				goto retry;
+			}
+		} else {
+			err = fuse_do_getattr(inode, stat, file);
+		}
 	} else if (stat) {
 		generic_fillattr(&nop_mnt_idmap, inode, stat);
 		stat->mode = fi->orig_i_mode;
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -792,6 +792,9 @@ struct fuse_conn {
 	/* Is tmpfile not implemented by fs? */
 	unsigned int no_tmpfile:1;
 
+	/* Is statx not implemented by fs? */
+	unsigned int no_statx:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -77,7 +77,7 @@ static struct inode *fuse_alloc_inode(st
 		return NULL;
 
 	fi->i_time = 0;
-	fi->inval_mask = 0;
+	fi->inval_mask = ~0;
 	fi->nodeid = 0;
 	fi->nlookup = 0;
 	fi->attr_version = 0;
@@ -172,7 +172,8 @@ void fuse_change_attributes_common(struc
 
 	fi->attr_version = atomic64_inc_return(&fc->attr_version);
 	fi->i_time = attr_valid;
-	WRITE_ONCE(fi->inval_mask, 0);
+	/* Clear basic stats from invalid mask */
+	set_mask_bits(&fi->inval_mask, STATX_BASIC_STATS, 0);
 
 	inode->i_ino     = fuse_squash_ino(attr->ino);
 	inode->i_mode    = (inode->i_mode & S_IFMT) | (attr->mode & 07777);

Re: [PATCH 2/5] fuse: add STATX request

[Bernd Schubert]

On 8/10/23 16:08, Miklos Szeredi wrote:
> On Thu, 10 Aug 2023 at 15:23, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/10/23 12:54, Miklos Szeredi wrote:
>>> Use the same structure as statx.
>>
>> Wouldn't it be easier to just include struct statx? Or is there an issue
>> with __u32, etc? If so, just a sufficiently large array could be used
>> and statx values just mem-copied in/out?
> 
> <linux/uapi/fuse.h> is OS independent.  Ports can grab it and use it
> in their userspace and kernel implementations.

Ok, but why not just like this?

struct fuse_statx {
	uint8_t fuse_statx_values[256];
}

struct fuse_statx fuse_statx;
struct statx *statx_ptr = (struct statx *)&fuse_statx.fuse_statx_values;


Hmm, I see an issue if that struct is passed over network and different 
OS are involved, which _might_ have different structs or sizes. So yeah, 
the copy approach is safest. Although as fuse_statx is between kernel 
and userspace only - it should not matter?

> 
> 
>>
>>>
>>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>> ---
>>>    include/uapi/linux/fuse.h | 56 ++++++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index b3fcab13fcd3..fe700b91b33b 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -207,6 +207,9 @@
>>>     *  - add FUSE_EXT_GROUPS
>>>     *  - add FUSE_CREATE_SUPP_GROUP
>>>     *  - add FUSE_HAS_EXPIRE_ONLY
>>> + *
>>> + *  7.39
>>> + *  - add FUSE_STATX and related structures
>>>     */
>>>
>>>    #ifndef _LINUX_FUSE_H
>>> @@ -242,7 +245,7 @@
>>>    #define FUSE_KERNEL_VERSION 7
>>>
>>>    /** Minor version number of this interface */
>>> -#define FUSE_KERNEL_MINOR_VERSION 38
>>> +#define FUSE_KERNEL_MINOR_VERSION 39
>>>
>>>    /** The node ID of the root inode */
>>>    #define FUSE_ROOT_ID 1
>>> @@ -269,6 +272,40 @@ struct fuse_attr {
>>>        uint32_t        flags;
>>>    };
>>>
>>> +/*
>>> + * The following structures are bit-for-bit compatible with the statx(2) ABI in
>>> + * Linux.
>>> + */
>>> +struct fuse_sx_time {
>>> +     int64_t         tv_sec;
>>> +     uint32_t        tv_nsec;
>>> +     int32_t         __reserved;
>>> +};
>>> +
>>> +struct fuse_statx {
>>> +     uint32_t        mask;
>>> +     uint32_t        blksize;
>>> +     uint64_t        attributes;
>>> +     uint32_t        nlink;
>>> +     uint32_t        uid;
>>> +     uint32_t        gid;
>>> +     uint16_t        mode;
>>> +     uint16_t        __spare0[1];
>>> +     uint64_t        ino;
>>> +     uint64_t        size;
>>> +     uint64_t        blocks;
>>> +     uint64_t        attributes_mask;
>>> +     struct fuse_sx_time     atime;
>>> +     struct fuse_sx_time     btime;
>>> +     struct fuse_sx_time     ctime;
>>> +     struct fuse_sx_time     mtime;
>>> +     uint32_t        rdev_major;
>>> +     uint32_t        rdev_minor;
>>> +     uint32_t        dev_major;
>>> +     uint32_t        dev_minor;
>>> +     uint64_t        __spare2[14];
>>> +};
>>
>> Looks like some recent values are missing?
> 
> It doesn't matter, since those parts are not used.
> 
>>
>>          /* 0x90 */
>>          __u64   stx_mnt_id;
>>          __u32   stx_dio_mem_align;      /* Memory buffer alignment for direct I/O */
>>          __u32   stx_dio_offset_align;   /* File offset alignment for direct I/O */
>>          /* 0xa0 */
>>          __u64   __spare3[12];   /* Spare space for future expansion */
>>          /* 0x100 */
>>
>> Which is basically why my personal preference would be not to do have a
>> copy of the struct - there is maintenance overhead.
> 
> Whenever the new fields would be used in the kernel the fields can be
> added.  So no need to continually update the one in fuse, since those

Maybe I'm over optimizing, I just see that userspace side then also 
needs an updated struct - which can be easily forgotten. While plain 
struct statx would't have that issue.



Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

Hi Miklos,

sorry for late review.

On 8/10/23 12:55, Miklos Szeredi wrote:
[...]
> +static int fuse_do_statx(struct inode *inode, struct file *file,
> +			 struct kstat *stat)
> +{
> +	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;
> +	}
> +	/* For now leave sync hints as the default, request all stats. */
> +	inarg.sx_flags = 0;
> +	inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;



What is actually the reason not to pass through flags from 
fuse_update_get_attr()? Wouldn't it make sense to request the minimal 
required mask and then server side can decide if it wants to fill in more?


Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
> Hi Miklos,
>
> sorry for late review.
>
> On 8/10/23 12:55, Miklos Szeredi wrote:
> [...]
> > +static int fuse_do_statx(struct inode *inode, struct file *file,
> > +                      struct kstat *stat)
> > +{
> > +     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;
> > +     }
> > +     /* For now leave sync hints as the default, request all stats. */
> > +     inarg.sx_flags = 0;
> > +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
>
>
>
> What is actually the reason not to pass through flags from
> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
> required mask and then server side can decide if it wants to fill in more?

This and following commit is about btime and btime only.  It's about
adding just this single attribute, otherwise the logic is unchanged.

But the flexibility is there in the interface definition, and
functionality can be added later.

Thanks,
Miklos

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

On 8/10/23 12:55, Miklos Szeredi wrote:
> Allow querying btime.  When btime is requested in mask, then FUSE_STATX
> request is sent.  Otherwise keep using FUSE_GETATTR.
> 
> The userspace interface for statx matches that of the statx(2) API.
> However there are limitations on how this interface is used:
> 
>   - returned basic stats and btime are used, stx_attributes, etc. are
>     ignored
> 
>   - always query basic stats and btime, regardless of what was requested
> 
>   - requested sync type is ignored, the default is passed to the server
> 
>   - if server returns with some attributes missing from the result_mask,
>     then no attributes will be cached
> 
>   - btime is not cached yet (next patch will fix that)
> 
> For new inodes initialize fi->inval_mask to "all invalid", instead of "all
> valid" as previously.  Also only clear basic stats from inval_mask when
> caching attributes.  This will result in the caching logic not thinking
> that btime is cached.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>   fs/fuse/dir.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++---
>   fs/fuse/fuse_i.h |   3 ++
>   fs/fuse/inode.c  |   5 ++-
>   3 files changed, 107 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 04006db6e173..552157bd6a4d 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -350,10 +350,14 @@ int fuse_valid_type(int m)
>   		S_ISBLK(m) || S_ISFIFO(m) || S_ISSOCK(m);
>   }
>   
> +bool fuse_valid_size(u64 size)
> +{
> +	return size <= LLONG_MAX;
> +}
> +
>   bool fuse_invalid_attr(struct fuse_attr *attr)
>   {
> -	return !fuse_valid_type(attr->mode) ||
> -		attr->size > LLONG_MAX;
> +	return !fuse_valid_type(attr->mode) || !fuse_valid_size(attr->size);
>   }
>   
>   int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name,
> @@ -1143,6 +1147,84 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
>   	stat->blksize = 1 << blkbits;
>   }
>   
> +static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
> +{
> +	memset(attr, 0, sizeof(*attr));
> +	attr->ino = sx->ino;
> +	attr->size = sx->size;
> +	attr->blocks = sx->blocks;
> +	attr->atime = sx->atime.tv_sec;
> +	attr->mtime = sx->mtime.tv_sec;
> +	attr->ctime = sx->ctime.tv_sec;
> +	attr->atimensec = sx->atime.tv_nsec;
> +	attr->mtimensec = sx->mtime.tv_nsec;
> +	attr->ctimensec = sx->ctime.tv_nsec;
> +	attr->mode = sx->mode;
> +	attr->nlink = sx->nlink;
> +	attr->uid = sx->uid;
> +	attr->gid = sx->gid;
> +	attr->rdev = new_encode_dev(MKDEV(sx->rdev_major, sx->rdev_minor));
> +	attr->blksize = sx->blksize;
> +}
> +
> +static int fuse_do_statx(struct inode *inode, struct file *file,
> +			 struct kstat *stat)
> +{
> +	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;
> +	}
> +	/* For now leave sync hints as the default, request all stats. */
> +	inarg.sx_flags = 0;
> +	inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
> +	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)))) {
> +		make_bad_inode(inode);
> +		return -EIO;
> +	}
> +
> +	fuse_statx_to_attr(&outarg.stat, &attr);
> +	if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
> +		fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
> +				       attr_version);
> +	}
> +	stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
> +	stat->btime.tv_sec = sx->btime.tv_sec;
> +	stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
> +	fuse_fillattr(inode, &attr, stat);
> +	stat->result_mask |= STATX_TYPE;
> +
> +	return 0;
> +}


Hmm, unconditionally using stat is potentially a NULL ptr with future 
updates. I think not right now, as fuse_update_get_attr() has the 
(request_mask & ~STATX_BASIC_STATS) condition and no caller
that passes 'stat = NULL' requests anything beyond STATX_BASIC_STATS, 
but wouldn't it be more safe to access stat only conditionally?


Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

On 8/22/23 17:33, Miklos Szeredi wrote:
> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>> Hi Miklos,
>>
>> sorry for late review.
>>
>> On 8/10/23 12:55, Miklos Szeredi wrote:
>> [...]
>>> +static int fuse_do_statx(struct inode *inode, struct file *file,
>>> +                      struct kstat *stat)
>>> +{
>>> +     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;
>>> +     }
>>> +     /* For now leave sync hints as the default, request all stats. */
>>> +     inarg.sx_flags = 0;
>>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
>>
>>
>>
>> What is actually the reason not to pass through flags from
>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
>> required mask and then server side can decide if it wants to fill in more?
> 
> This and following commit is about btime and btime only.  It's about
> adding just this single attribute, otherwise the logic is unchanged.
> 
> But the flexibility is there in the interface definition, and
> functionality can be added later.

Sure, though what speaks against setting (limiting the mask) right away?

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 42f49fe6e770..de1d991757a5 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1168,7 +1168,8 @@ 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, u32 request_mask,
+                        unsigned int flags)
  {
         int err;
         struct fuse_attr attr;
@@ -1188,9 +1189,10 @@ static int fuse_do_statx(struct inode *inode, struct file *file,
                 inarg.getattr_flags |= FUSE_GETATTR_FH;
                 inarg.fh = ff->fh;
         }
-       /* For now leave sync hints as the default, request all stats. */
-       inarg.sx_flags = 0;
-       inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
+
+       /* request the given mask, server side is free to return more */
+       inarg.sx_flags = flags;
+       inarg.sx_mask = request_mask;
         args.opcode = FUSE_STATX;
         args.nodeid = get_node_id(inode);
         args.in_numargs = 1;
@@ -1304,7 +1306,8 @@ static int fuse_update_get_attr(struct inode *inode, struct file *file,
                 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);
+                       err = fuse_do_statx(inode, file, stat, request_mask,
+                                           flags);
                         if (err == -ENOSYS) {
                                 fc->no_statx = 1;
                                 goto retry;



Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

On Tue, 22 Aug 2023 at 18:39, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/10/23 12:55, Miklos Szeredi wrote:
> > Allow querying btime.  When btime is requested in mask, then FUSE_STATX
> > request is sent.  Otherwise keep using FUSE_GETATTR.
> >
> > The userspace interface for statx matches that of the statx(2) API.
> > However there are limitations on how this interface is used:
> >
> >   - returned basic stats and btime are used, stx_attributes, etc. are
> >     ignored
> >
> >   - always query basic stats and btime, regardless of what was requested
> >
> >   - requested sync type is ignored, the default is passed to the server
> >
> >   - if server returns with some attributes missing from the result_mask,
> >     then no attributes will be cached
> >
> >   - btime is not cached yet (next patch will fix that)
> >
> > For new inodes initialize fi->inval_mask to "all invalid", instead of "all
> > valid" as previously.  Also only clear basic stats from inval_mask when
> > caching attributes.  This will result in the caching logic not thinking
> > that btime is cached.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >   fs/fuse/dir.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++---
> >   fs/fuse/fuse_i.h |   3 ++
> >   fs/fuse/inode.c  |   5 ++-
> >   3 files changed, 107 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 04006db6e173..552157bd6a4d 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -350,10 +350,14 @@ int fuse_valid_type(int m)
> >               S_ISBLK(m) || S_ISFIFO(m) || S_ISSOCK(m);
> >   }
> >
> > +bool fuse_valid_size(u64 size)
> > +{
> > +     return size <= LLONG_MAX;
> > +}
> > +
> >   bool fuse_invalid_attr(struct fuse_attr *attr)
> >   {
> > -     return !fuse_valid_type(attr->mode) ||
> > -             attr->size > LLONG_MAX;
> > +     return !fuse_valid_type(attr->mode) || !fuse_valid_size(attr->size);
> >   }
> >
> >   int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name,
> > @@ -1143,6 +1147,84 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
> >       stat->blksize = 1 << blkbits;
> >   }
> >
> > +static void fuse_statx_to_attr(struct fuse_statx *sx, struct fuse_attr *attr)
> > +{
> > +     memset(attr, 0, sizeof(*attr));
> > +     attr->ino = sx->ino;
> > +     attr->size = sx->size;
> > +     attr->blocks = sx->blocks;
> > +     attr->atime = sx->atime.tv_sec;
> > +     attr->mtime = sx->mtime.tv_sec;
> > +     attr->ctime = sx->ctime.tv_sec;
> > +     attr->atimensec = sx->atime.tv_nsec;
> > +     attr->mtimensec = sx->mtime.tv_nsec;
> > +     attr->ctimensec = sx->ctime.tv_nsec;
> > +     attr->mode = sx->mode;
> > +     attr->nlink = sx->nlink;
> > +     attr->uid = sx->uid;
> > +     attr->gid = sx->gid;
> > +     attr->rdev = new_encode_dev(MKDEV(sx->rdev_major, sx->rdev_minor));
> > +     attr->blksize = sx->blksize;
> > +}
> > +
> > +static int fuse_do_statx(struct inode *inode, struct file *file,
> > +                      struct kstat *stat)
> > +{
> > +     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;
> > +     }
> > +     /* For now leave sync hints as the default, request all stats. */
> > +     inarg.sx_flags = 0;
> > +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
> > +     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)))) {
> > +             make_bad_inode(inode);
> > +             return -EIO;
> > +     }
> > +
> > +     fuse_statx_to_attr(&outarg.stat, &attr);
> > +     if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
> > +             fuse_change_attributes(inode, &attr, ATTR_TIMEOUT(&outarg),
> > +                                    attr_version);
> > +     }
> > +     stat->result_mask = sx->mask & (STATX_BASIC_STATS | STATX_BTIME);
> > +     stat->btime.tv_sec = sx->btime.tv_sec;
> > +     stat->btime.tv_nsec = min_t(u32, sx->btime.tv_nsec, NSEC_PER_SEC - 1);
> > +     fuse_fillattr(inode, &attr, stat);
> > +     stat->result_mask |= STATX_TYPE;
> > +
> > +     return 0;
> > +}
>
>
> Hmm, unconditionally using stat is potentially a NULL ptr with future
> updates. I think not right now, as fuse_update_get_attr() has the
> (request_mask & ~STATX_BASIC_STATS) condition and no caller
> that passes 'stat = NULL' requests anything beyond STATX_BASIC_STATS,
> but wouldn't it be more safe to access stat only conditionally?

Yes, makes sense.

Thanks,
Miklos

Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/22/23 17:33, Miklos Szeredi wrote:
> > On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
> >>
> >> Hi Miklos,
> >>
> >> sorry for late review.
> >>
> >> On 8/10/23 12:55, Miklos Szeredi wrote:
> >> [...]
> >>> +static int fuse_do_statx(struct inode *inode, struct file *file,
> >>> +                      struct kstat *stat)
> >>> +{
> >>> +     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;
> >>> +     }
> >>> +     /* For now leave sync hints as the default, request all stats. */
> >>> +     inarg.sx_flags = 0;
> >>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
> >>
> >>
> >>
> >> What is actually the reason not to pass through flags from
> >> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
> >> required mask and then server side can decide if it wants to fill in more?
> >
> > This and following commit is about btime and btime only.  It's about
> > adding just this single attribute, otherwise the logic is unchanged.
> >
> > But the flexibility is there in the interface definition, and
> > functionality can be added later.
>
> Sure, though what speaks against setting (limiting the mask) right away?

But then the result is basically uncacheable, until we have separate
validity timeouts for each attribute.  Maybe we need that, maybe not,
but it does definitely have side effects.

Thanks,
Miklos

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

On 8/23/23 08:18, Miklos Szeredi wrote:
> On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/22/23 17:33, Miklos Szeredi wrote:
>>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>
>>>> Hi Miklos,
>>>>
>>>> sorry for late review.
>>>>
>>>> On 8/10/23 12:55, Miklos Szeredi wrote:
>>>> [...]
>>>>> +static int fuse_do_statx(struct inode *inode, struct file *file,
>>>>> +                      struct kstat *stat)
>>>>> +{
>>>>> +     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;
>>>>> +     }
>>>>> +     /* For now leave sync hints as the default, request all stats. */
>>>>> +     inarg.sx_flags = 0;
>>>>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
>>>>
>>>>
>>>>
>>>> What is actually the reason not to pass through flags from
>>>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
>>>> required mask and then server side can decide if it wants to fill in more?
>>>
>>> This and following commit is about btime and btime only.  It's about
>>> adding just this single attribute, otherwise the logic is unchanged.
>>>
>>> But the flexibility is there in the interface definition, and
>>> functionality can be added later.
>>
>> Sure, though what speaks against setting (limiting the mask) right away?
> 
> But then the result is basically uncacheable, until we have separate
> validity timeouts for each attribute.  Maybe we need that, maybe not,
> but it does definitely have side effects.

Ah right, updating the cache timeout shouldn't be done unless the reply 
contains all attributes. Although you already handle that in fuse_do_statx


	if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
		fuse_change_attributes(inode, &attr, &outarg.stat,
				       ATTR_TIMEOUT(&outarg), attr_version);
	}



Thanks,
Bernd

Re: [PATCH 4/5] fuse: implement statx

[Miklos Szeredi]

On Wed, 23 Aug 2023 at 16:51, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/23/23 08:18, Miklos Szeredi wrote:
> > On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
> >>
> >>
> >>
> >> On 8/22/23 17:33, Miklos Szeredi wrote:
> >>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
> >>>>
> >>>> Hi Miklos,
> >>>>
> >>>> sorry for late review.
> >>>>
> >>>> On 8/10/23 12:55, Miklos Szeredi wrote:
> >>>> [...]
> >>>>> +static int fuse_do_statx(struct inode *inode, struct file *file,
> >>>>> +                      struct kstat *stat)
> >>>>> +{
> >>>>> +     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;
> >>>>> +     }
> >>>>> +     /* For now leave sync hints as the default, request all stats. */
> >>>>> +     inarg.sx_flags = 0;
> >>>>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
> >>>>
> >>>>
> >>>>
> >>>> What is actually the reason not to pass through flags from
> >>>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
> >>>> required mask and then server side can decide if it wants to fill in more?
> >>>
> >>> This and following commit is about btime and btime only.  It's about
> >>> adding just this single attribute, otherwise the logic is unchanged.
> >>>
> >>> But the flexibility is there in the interface definition, and
> >>> functionality can be added later.
> >>
> >> Sure, though what speaks against setting (limiting the mask) right away?
> >
> > But then the result is basically uncacheable, until we have separate
> > validity timeouts for each attribute.  Maybe we need that, maybe not,
> > but it does definitely have side effects.
>
> Ah right, updating the cache timeout shouldn't be done unless the reply
> contains all attributes. Although you already handle that in fuse_do_statx

Yes, correctness is guaranteed.

However not setting the full mask might easily result in a performance
regression. At this point just avoid such issues by not allowing
partial masks to reach the server.

Thanks,
Miklos


>
>
>         if ((sx->mask & STATX_BASIC_STATS) == STATX_BASIC_STATS) {
>                 fuse_change_attributes(inode, &attr, &outarg.stat,
>                                        ATTR_TIMEOUT(&outarg), attr_version);
>         }
>
>
>
> Thanks,
> Bernd

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

On 8/23/23 16:58, Miklos Szeredi wrote:
> On Wed, 23 Aug 2023 at 16:51, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/23/23 08:18, Miklos Szeredi wrote:
>>> On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>
>>>>
>>>>
>>>> On 8/22/23 17:33, Miklos Szeredi wrote:
>>>>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>>>
>>>>>> Hi Miklos,
>>>>>>
>>>>>> sorry for late review.
>>>>>>
>>>>>> On 8/10/23 12:55, Miklos Szeredi wrote:
>>>>>> [...]
>>>>>>> +static int fuse_do_statx(struct inode *inode, struct file *file,
>>>>>>> +                      struct kstat *stat)
>>>>>>> +{
>>>>>>> +     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;
>>>>>>> +     }
>>>>>>> +     /* For now leave sync hints as the default, request all stats. */
>>>>>>> +     inarg.sx_flags = 0;
>>>>>>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
>>>>>>
>>>>>>
>>>>>>
>>>>>> What is actually the reason not to pass through flags from
>>>>>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
>>>>>> required mask and then server side can decide if it wants to fill in more?
>>>>>
>>>>> This and following commit is about btime and btime only.  It's about
>>>>> adding just this single attribute, otherwise the logic is unchanged.
>>>>>
>>>>> But the flexibility is there in the interface definition, and
>>>>> functionality can be added later.
>>>>
>>>> Sure, though what speaks against setting (limiting the mask) right away?
>>>
>>> But then the result is basically uncacheable, until we have separate
>>> validity timeouts for each attribute.  Maybe we need that, maybe not,
>>> but it does definitely have side effects.
>>
>> Ah right, updating the cache timeout shouldn't be done unless the reply
>> contains all attributes. Although you already handle that in fuse_do_statx
> 
> Yes, correctness is guaranteed.
> 
> However not setting the full mask might easily result in a performance
> regression. At this point just avoid such issues by not allowing
> partial masks to reach the server.


Hmm ok, I see a bit more like "these flags are absolutely needed, if you 
(daemon/server) can provide more/all, I can update my cache timeout".

Re: [PATCH 4/5] fuse: implement statx

[Bernd Schubert]

On 8/23/23 16:58, Miklos Szeredi wrote:
> On Wed, 23 Aug 2023 at 16:51, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>>
>>
>> On 8/23/23 08:18, Miklos Szeredi wrote:
>>> On Tue, 22 Aug 2023 at 18:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>
>>>>
>>>>
>>>> On 8/22/23 17:33, Miklos Szeredi wrote:
>>>>> On Tue, 22 Aug 2023 at 17:20, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>>>>>
>>>>>> Hi Miklos,
>>>>>>
>>>>>> sorry for late review.
>>>>>>
>>>>>> On 8/10/23 12:55, Miklos Szeredi wrote:
>>>>>> [...]
>>>>>>> +static int fuse_do_statx(struct inode *inode, struct file *file,
>>>>>>> +                      struct kstat *stat)
>>>>>>> +{
>>>>>>> +     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;
>>>>>>> +     }
>>>>>>> +     /* For now leave sync hints as the default, request all stats. */
>>>>>>> +     inarg.sx_flags = 0;
>>>>>>> +     inarg.sx_mask = STATX_BASIC_STATS | STATX_BTIME;
>>>>>>
>>>>>>
>>>>>>
>>>>>> What is actually the reason not to pass through flags from
>>>>>> fuse_update_get_attr()? Wouldn't it make sense to request the minimal
>>>>>> required mask and then server side can decide if it wants to fill in more?
>>>>>
>>>>> This and following commit is about btime and btime only.  It's about
>>>>> adding just this single attribute, otherwise the logic is unchanged.
>>>>>
>>>>> But the flexibility is there in the interface definition, and
>>>>> functionality can be added later.
>>>>
>>>> Sure, though what speaks against setting (limiting the mask) right away?
>>>
>>> But then the result is basically uncacheable, until we have separate
>>> validity timeouts for each attribute.  Maybe we need that, maybe not,
>>> but it does definitely have side effects.
>>
>> Ah right, updating the cache timeout shouldn't be done unless the reply
>> contains all attributes. Although you already handle that in fuse_do_statx
> 
> Yes, correctness is guaranteed.
> 
> However not setting the full mask might easily result in a performance
> regression. At this point just avoid such issues by not allowing
> partial masks to reach the server.


Hmm ok, I see a bit more like "these flags are absolutely needed, if you 
(daemon/server) can provide more/all, I can update my cache timeout".


Thanks,
Bernd