[PATCH] ext4: Add statx support

[PATCH] ext4: Add statx support

[David Howells]

Return enhanced file attributes from the Ext4 filesystem.  This includes
the following:

 (1) The inode creation time (i_crtime) as stx_btime, setting STATX_BTIME.

 (2) Certain FS_xxx_FL flags are mapped to stx_attribute flags.

This requires that all ext4 inodes have a getattr call, not just some of
them, so to this end, split the ext4_getattr() function and only call part
of it where appropriate.

Example output:

	[root@andromeda ~]# touch foo
	[root@andromeda ~]# chattr +ai foo
	[root@andromeda ~]# /tmp/test-statx foo
	statx(foo) = 0
	results=fff
	  Size: 0               Blocks: 0          IO Block: 4096    regular file
	Device: 08:12           Inode: 2101950     Links: 1
	Access: (0644/-rw-r--r--)  Uid:     0   Gid:     0
	Access: 2016-02-11 17:08:29.031795451+0000
	Modify: 2016-02-11 17:08:29.031795451+0000
	Change: 2016-02-11 17:11:11.987790114+0000
	 Birth: 2016-02-11 17:08:29.031795451+0000
	Attributes: 0000000000000030 (-------- -------- -------- -------- -------- -------- -------- --ai----)

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/ext4/ext4.h    |    1 +
 fs/ext4/file.c    |    2 +-
 fs/ext4/inode.c   |   36 +++++++++++++++++++++++++++++++++---
 fs/ext4/namei.c   |    2 ++
 fs/ext4/symlink.c |    2 ++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f493af666591..fb69ee2388db 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2466,6 +2466,7 @@ extern int  ext4_setattr(struct dentry *, struct iattr *);
 extern int  ext4_getattr(const struct path *, struct kstat *, u32, unsigned int);
 extern void ext4_evict_inode(struct inode *);
 extern void ext4_clear_inode(struct inode *);
+extern int  ext4_file_getattr(const struct path *, struct kstat *, u32, unsigned int);
 extern int  ext4_sync_inode(handle_t *, struct inode *);
 extern void ext4_dirty_inode(struct inode *, int);
 extern int ext4_change_inode_journal_flag(struct inode *, int);
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8210c1f43556..cefa9835f275 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -744,7 +744,7 @@ const struct file_operations ext4_file_operations = {
 
 const struct inode_operations ext4_file_inode_operations = {
 	.setattr	= ext4_setattr,
-	.getattr	= ext4_getattr,
+	.getattr	= ext4_file_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7385e6a6b6cb..5450e1eade1d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5390,11 +5390,41 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 int ext4_getattr(const struct path *path, struct kstat *stat,
 		 u32 request_mask, unsigned int query_flags)
 {
-	struct inode *inode;
-	unsigned long long delalloc_blocks;
+	struct inode *inode = d_inode(path->dentry);
+	struct ext4_inode *raw_inode;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	unsigned int flags;
+
+	if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
+		stat->result_mask |= STATX_BTIME;
+		stat->btime.tv_sec = ei->i_crtime.tv_sec;
+		stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
+	}
+
+	ext4_get_inode_flags(ei);
+	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
+	if (flags & EXT4_APPEND_FL)
+		stat->attributes |= STATX_ATTR_APPEND;
+	if (flags & EXT4_COMPR_FL)
+		stat->attributes |= STATX_ATTR_COMPRESSED;
+	if (flags & EXT4_ENCRYPT_FL)
+		stat->attributes |= STATX_ATTR_ENCRYPTED;
+	if (flags & EXT4_IMMUTABLE_FL)
+		stat->attributes |= STATX_ATTR_IMMUTABLE;
+	if (flags & EXT4_NODUMP_FL)
+		stat->attributes |= STATX_ATTR_NODUMP;
 
-	inode = d_inode(path->dentry);
 	generic_fillattr(inode, stat);
+	return 0;
+}
+
+int ext4_file_getattr(const struct path *path, struct kstat *stat,
+		      u32 request_mask, unsigned int query_flags)
+{
+	struct inode *inode = d_inode(path->dentry);
+	u64 delalloc_blocks;
+
+	ext4_getattr(path, stat, request_mask, query_flags);
 
 	/*
 	 * If there is inline data in the inode, the inode will normally not
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 6ad612c576fc..07e5e1405771 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3912,6 +3912,7 @@ const struct inode_operations ext4_dir_inode_operations = {
 	.tmpfile	= ext4_tmpfile,
 	.rename		= ext4_rename2,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
@@ -3920,6 +3921,7 @@ const struct inode_operations ext4_dir_inode_operations = {
 
 const struct inode_operations ext4_special_inode_operations = {
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
index 73b184d161fc..4c6b23a0603e 100644
--- a/fs/ext4/symlink.c
+++ b/fs/ext4/symlink.c
@@ -91,11 +91,13 @@ const struct inode_operations ext4_encrypted_symlink_inode_operations = {
 const struct inode_operations ext4_symlink_inode_operations = {
 	.get_link	= page_get_link,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 };
 
 const struct inode_operations ext4_fast_symlink_inode_operations = {
 	.get_link	= simple_get_link,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 };

Re: [PATCH] ext4: Add statx support

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5885 bytes --]

On Mar 16, 2017, at 5:35 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Return enhanced file attributes from the Ext4 filesystem.  This includes
> the following:
> 
> (1) The inode creation time (i_crtime) as stx_btime, setting STATX_BTIME.
> 
> (2) Certain FS_xxx_FL flags are mapped to stx_attribute flags.
> 
> This requires that all ext4 inodes have a getattr call, not just some of
> them, so to this end, split the ext4_getattr() function and only call part
> of it where appropriate.
> 
> Example output:
> 
> 	[root@andromeda ~]# touch foo
> 	[root@andromeda ~]# chattr +ai foo
> 	[root@andromeda ~]# /tmp/test-statx foo
> 	statx(foo) = 0
> 	results=fff
> 	  Size: 0               Blocks: 0          IO Block: 4096    regular file
> 	Device: 08:12           Inode: 2101950     Links: 1
> 	Access: (0644/-rw-r--r--)  Uid:     0   Gid:     0
> 	Access: 2016-02-11 17:08:29.031795451+0000
> 	Modify: 2016-02-11 17:08:29.031795451+0000
> 	Change: 2016-02-11 17:11:11.987790114+0000
> 	 Birth: 2016-02-11 17:08:29.031795451+0000
> 	Attributes: 0000000000000030 (-------- -------- -------- -------- -------- -------- -------- --ai----)
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> 
> fs/ext4/ext4.h    |    1 +
> fs/ext4/file.c    |    2 +-
> fs/ext4/inode.c   |   36 +++++++++++++++++++++++++++++++++---
> fs/ext4/namei.c   |    2 ++
> fs/ext4/symlink.c |    2 ++
> 5 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index f493af666591..fb69ee2388db 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2466,6 +2466,7 @@ extern int  ext4_setattr(struct dentry *, struct iattr *);
> extern int  ext4_getattr(const struct path *, struct kstat *, u32, unsigned int);
> extern void ext4_evict_inode(struct inode *);
> extern void ext4_clear_inode(struct inode *);
> +extern int  ext4_file_getattr(const struct path *, struct kstat *, u32, unsigned int);

(style) should this wrap at 80 columns?

> extern int  ext4_sync_inode(handle_t *, struct inode *);
> extern void ext4_dirty_inode(struct inode *, int);
> extern int ext4_change_inode_journal_flag(struct inode *, int);
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 8210c1f43556..cefa9835f275 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -744,7 +744,7 @@ const struct file_operations ext4_file_operations = {
> 
> const struct inode_operations ext4_file_inode_operations = {
> 	.setattr	= ext4_setattr,
> -	.getattr	= ext4_getattr,
> +	.getattr	= ext4_file_getattr,
> 	.listxattr	= ext4_listxattr,
> 	.get_acl	= ext4_get_acl,
> 	.set_acl	= ext4_set_acl,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 7385e6a6b6cb..5450e1eade1d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5390,11 +5390,41 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> int ext4_getattr(const struct path *path, struct kstat *stat,
> 		 u32 request_mask, unsigned int query_flags)
> {
> -	struct inode *inode;
> -	unsigned long long delalloc_blocks;
> +	struct inode *inode = d_inode(path->dentry);
> +	struct ext4_inode *raw_inode;
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	unsigned int flags;
> +
> +	if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
> +		stat->result_mask |= STATX_BTIME;
> +		stat->btime.tv_sec = ei->i_crtime.tv_sec;
> +		stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
> +	}
> +
> +	ext4_get_inode_flags(ei);
> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
> +	if (flags & EXT4_APPEND_FL)
> +		stat->attributes |= STATX_ATTR_APPEND;
> +	if (flags & EXT4_COMPR_FL)
> +		stat->attributes |= STATX_ATTR_COMPRESSED;
> +	if (flags & EXT4_ENCRYPT_FL)
> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
> +	if (flags & EXT4_IMMUTABLE_FL)
> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> +	if (flags & EXT4_NODUMP_FL)
> +		stat->attributes |= STATX_ATTR_NODUMP;
> 
> -	inode = d_inode(path->dentry);
> 	generic_fillattr(inode, stat);
> +	return 0;
> +}
> +
> +int ext4_file_getattr(const struct path *path, struct kstat *stat,
> +		      u32 request_mask, unsigned int query_flags)
> +{
> +	struct inode *inode = d_inode(path->dentry);
> +	u64 delalloc_blocks;
> +
> +	ext4_getattr(path, stat, request_mask, query_flags);
> 
> 	/*
> 	 * If there is inline data in the inode, the inode will normally not
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 6ad612c576fc..07e5e1405771 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3912,6 +3912,7 @@ const struct inode_operations ext4_dir_inode_operations = {
> 	.tmpfile	= ext4_tmpfile,
> 	.rename		= ext4_rename2,
> 	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
> 	.listxattr	= ext4_listxattr,
> 	.get_acl	= ext4_get_acl,
> 	.set_acl	= ext4_set_acl,
> @@ -3920,6 +3921,7 @@ const struct inode_operations ext4_dir_inode_operations = {
> 
> const struct inode_operations ext4_special_inode_operations = {
> 	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
> 	.listxattr	= ext4_listxattr,
> 	.get_acl	= ext4_get_acl,
> 	.set_acl	= ext4_set_acl,
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 73b184d161fc..4c6b23a0603e 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -91,11 +91,13 @@ const struct inode_operations ext4_encrypted_symlink_inode_operations = {
> const struct inode_operations ext4_symlink_inode_operations = {
> 	.get_link	= page_get_link,
> 	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
> 	.listxattr	= ext4_listxattr,
> };
> 
> const struct inode_operations ext4_fast_symlink_inode_operations = {
> 	.get_link	= simple_get_link,
> 	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
> 	.listxattr	= ext4_listxattr,
> };
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ext4: Add statx support

[Eric Biggers]

Hi David,

On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote:
> +
> +	ext4_get_inode_flags(ei);
> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
> +	if (flags & EXT4_APPEND_FL)
> +		stat->attributes |= STATX_ATTR_APPEND;
> +	if (flags & EXT4_COMPR_FL)
> +		stat->attributes |= STATX_ATTR_COMPRESSED;
> +	if (flags & EXT4_ENCRYPT_FL)
> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
> +	if (flags & EXT4_IMMUTABLE_FL)
> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> +	if (flags & EXT4_NODUMP_FL)
> +		stat->attributes |= STATX_ATTR_NODUMP;
>  
> -	inode = d_inode(path->dentry);
>  	generic_fillattr(inode, stat);
> +	return 0;
> +}

I think it's the wrong approach to be calling ext4_get_inode_flags() here to
sync the generic inode flags (inode->i_flags) to the ext4-specific inode flags
(ei->i_flags).  I know the GETFLAGS and FSGETXATTR ioctls do it too, but I think
it's a mistake --- at least, when done so without holding the inode lock.  The
issue is that flag syncs can occur in both directions concurrently and cause an
update to be lost.  For example, with thread 1 doing a stat() and thread 2 doing
the SETFLAGS ioctl to set the APPEND flag:

	Thread 1, in ext4_get_inode_flags()	Thread 2, in ext4_ioctl_setflags()
	-----------------------------------    -----------------------------------

	Read inode->i_flags; S_APPEND is clear	
						Set EXT4_APPEND_FL in ei->i_flags
	Clear EXT4_APPEND_FL in ei->i_flags
						Read ei->i_flags; EXT4_APPEND_FL is clear
						Clear S_APPEND in inode->i_flags

So the flag set by SETFLAGS gets lost.  This bug probably hasn't really been
noticable with GETFLAGS and FSGETXATTR since they're rarely used, but stat() on
the other hand is very common, and I'm worried that with this change people
would start seeing this race more often.

Ultimately this needs to be addressed in ext4 more fully, but how about for
->getattr() just skipping the call to ext4_get_inode_flags() and instead
populating the generic attributes like STATX_ATTR_APPEND and
STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the
ext4-specific flags?  Actually, it could even be done in generic_fillattr(), so
that all filesystems benefit.

> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 73b184d161fc..4c6b23a0603e 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -91,11 +91,13 @@ const struct inode_operations ext4_encrypted_symlink_inode_operations = {
>  const struct inode_operations ext4_symlink_inode_operations = {
>  	.get_link	= page_get_link,
>  	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
>  	.listxattr	= ext4_listxattr,
>  };
>  
>  const struct inode_operations ext4_fast_symlink_inode_operations = {
>  	.get_link	= simple_get_link,
>  	.setattr	= ext4_setattr,
> +	.getattr	= ext4_getattr,
>  	.listxattr	= ext4_listxattr,
>  };
> 

getattr needs to be added to ext4_encrypted_symlink_inode_operations too.

- Eric

Re: [PATCH] ext4: Add statx support

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4318 bytes --]

On Mar 16, 2017, at 11:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote:
>> +
>> +	ext4_get_inode_flags(ei);
>> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
>> +	if (flags & EXT4_APPEND_FL)
>> +		stat->attributes |= STATX_ATTR_APPEND;
>> +	if (flags & EXT4_COMPR_FL)
>> +		stat->attributes |= STATX_ATTR_COMPRESSED;
>> +	if (flags & EXT4_ENCRYPT_FL)
>> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
>> +	if (flags & EXT4_IMMUTABLE_FL)
>> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
>> +	if (flags & EXT4_NODUMP_FL)
>> +		stat->attributes |= STATX_ATTR_NODUMP;
>> 
>> -	inode = d_inode(path->dentry);
>> 	generic_fillattr(inode, stat);
>> +	return 0;
>> +}
> 
> I think it's the wrong approach to be calling ext4_get_inode_flags() here to
> sync the generic inode flags (inode->i_flags) to the ext4-specific inode flags
> (ei->i_flags).  I know the GETFLAGS and FSGETXATTR ioctls do it too, but I
> think it's a mistake --- at least, when done so without holding the inode lock.
> The issue is that flag syncs can occur in both directions concurrently and
> cause an update to be lost.  For example, with thread 1 doing a stat() and
> thread 2 doing the SETFLAGS ioctl to set the APPEND flag:
> 
> Thread 1, ext4_get_inode_flags()	Thread 2, ext4_ioctl_setflags()
> -----------------------------------	---------------------------
> Read inode->i_flags; S_APPEND clear
> 					Set EXT4_APPEND_FL in ei->i_flags
> Clear EXT4_APPEND_FL in ei->i_flags
> 					Read ei->i_flags; EXT4_APPEND_FL clear
> 					Clear S_APPEND in inode->i_flags
> 
> So the flag set by SETFLAGS gets lost.  This bug probably hasn't really been
> noticable with GETFLAGS and FSGETXATTR since they're rarely used, but stat() on
> the other hand is very common, and I'm worried that with this change people
> would start seeing this race more often.
> 
> Ultimately this needs to be addressed in ext4 more fully, but how about for
> ->getattr() just skipping the call to ext4_get_inode_flags() and instead
> populating the generic attributes like STATX_ATTR_APPEND and
> STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the
> ext4-specific flags?  Actually, it could even be done in generic_fillattr(), so
> that all filesystems benefit.

Wouldn't it make more sense to just extract the ext4 flags directly from
ei->i_flags?  I think ext4_get_inode_flags() is only really useful when
the VFS inode flags are changed and they need to be propagated to the ext4
inode.

I guess the other more significant question is when/where are the VFS inode
flags changed that they need to be propagated into the ext4 disk inode?
The main avenue for changing these attribute flags that I know about is via
EXT4_IOC_SETFLAGS (FS_IOC_SETFLAGS), and there is one place that I could
find in fs/nsfs.c that sets S_IMMUTABLE but I don't think that propagates
down to an ext4 disk inode.

It seems like the use of ext4_get_inode_flags() is largely superfluous.
The original commit ff9ddf7e847 indicates this was for quota inodes only.
I think it can be removed from EXT4_IOC_FSGETXATTR, and EXT4_IOC_GETFLAGS
and made conditional in ext4_do_update_inode():

#ifdef CONFIG_QUOTA
        for (i = 0; i < EXT4_MAXQUOTAS; i++) {
                if (unlikely(sb_dqopt(sb)->files[i] == inode))
                        ext4_get_inode_flags(EXT4_I(inode));
        }
#endif

Jan, what do you think?

Cheers, Andreas

> 
>> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
>> index 73b184d161fc..4c6b23a0603e 100644
>> --- a/fs/ext4/symlink.c
>> +++ b/fs/ext4/symlink.c
>> @@ -91,11 +91,13 @@ const struct inode_operations ext4_encrypted_symlink_inode_operations = {
>> const struct inode_operations ext4_symlink_inode_operations = {
>> 	.get_link	= page_get_link,
>> 	.setattr	= ext4_setattr,
>> +	.getattr	= ext4_getattr,
>> 	.listxattr	= ext4_listxattr,
>> };
>> 
>> const struct inode_operations ext4_fast_symlink_inode_operations = {
>> 	.get_link	= simple_get_link,
>> 	.setattr	= ext4_setattr,
>> +	.getattr	= ext4_getattr,
>> 	.listxattr	= ext4_listxattr,
>> };
>> 
> 
> getattr needs to be added to ext4_encrypted_symlink_inode_operations too.
> 
> - Eric


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ext4: Add statx support

[Jan Kara]

On Fri 17-03-17 14:19:22, Andreas Dilger wrote:
> On Mar 16, 2017, at 11:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote:
> >> +
> >> +	ext4_get_inode_flags(ei);
> >> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
> >> +	if (flags & EXT4_APPEND_FL)
> >> +		stat->attributes |= STATX_ATTR_APPEND;
> >> +	if (flags & EXT4_COMPR_FL)
> >> +		stat->attributes |= STATX_ATTR_COMPRESSED;
> >> +	if (flags & EXT4_ENCRYPT_FL)
> >> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
> >> +	if (flags & EXT4_IMMUTABLE_FL)
> >> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> >> +	if (flags & EXT4_NODUMP_FL)
> >> +		stat->attributes |= STATX_ATTR_NODUMP;
> >> 
> >> -	inode = d_inode(path->dentry);
> >> 	generic_fillattr(inode, stat);
> >> +	return 0;
> >> +}
> > 
> > I think it's the wrong approach to be calling ext4_get_inode_flags() here to
> > sync the generic inode flags (inode->i_flags) to the ext4-specific inode flags
> > (ei->i_flags).  I know the GETFLAGS and FSGETXATTR ioctls do it too, but I
> > think it's a mistake --- at least, when done so without holding the inode lock.
> > The issue is that flag syncs can occur in both directions concurrently and
> > cause an update to be lost.  For example, with thread 1 doing a stat() and
> > thread 2 doing the SETFLAGS ioctl to set the APPEND flag:
> > 
> > Thread 1, ext4_get_inode_flags()	Thread 2, ext4_ioctl_setflags()
> > -----------------------------------	---------------------------
> > Read inode->i_flags; S_APPEND clear
> > 					Set EXT4_APPEND_FL in ei->i_flags
> > Clear EXT4_APPEND_FL in ei->i_flags
> > 					Read ei->i_flags; EXT4_APPEND_FL clear
> > 					Clear S_APPEND in inode->i_flags
> > 
> > So the flag set by SETFLAGS gets lost.  This bug probably hasn't really been
> > noticable with GETFLAGS and FSGETXATTR since they're rarely used, but stat() on
> > the other hand is very common, and I'm worried that with this change people
> > would start seeing this race more often.
> > 
> > Ultimately this needs to be addressed in ext4 more fully, but how about for
> > ->getattr() just skipping the call to ext4_get_inode_flags() and instead
> > populating the generic attributes like STATX_ATTR_APPEND and
> > STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the
> > ext4-specific flags?  Actually, it could even be done in generic_fillattr(), so
> > that all filesystems benefit.
> 
> Wouldn't it make more sense to just extract the ext4 flags directly from
> ei->i_flags?  I think ext4_get_inode_flags() is only really useful when
> the VFS inode flags are changed and they need to be propagated to the ext4
> inode.
> 
> I guess the other more significant question is when/where are the VFS inode
> flags changed that they need to be propagated into the ext4 disk inode?
> The main avenue for changing these attribute flags that I know about is via
> EXT4_IOC_SETFLAGS (FS_IOC_SETFLAGS), and there is one place that I could
> find in fs/nsfs.c that sets S_IMMUTABLE but I don't think that propagates
> down to an ext4 disk inode.

Yes, you seem to be right. And actually I have checked and XFS does not
bother to copy inode->i_flags to its on-disk flags so it seems generally we
are not expected to reflect inode->i_flags in on-disk state.

> It seems like the use of ext4_get_inode_flags() is largely superfluous.
> The original commit ff9ddf7e847 indicates this was for quota inodes only.
> I think it can be removed from EXT4_IOC_FSGETXATTR, and EXT4_IOC_GETFLAGS
> and made conditional in ext4_do_update_inode():
> 
> #ifdef CONFIG_QUOTA
>         for (i = 0; i < EXT4_MAXQUOTAS; i++) {
>                 if (unlikely(sb_dqopt(sb)->files[i] == inode))
>                         ext4_get_inode_flags(EXT4_I(inode));
>         }
> #endif
> 
> Jan, what do you think?

So I think even better would be to have ext4_quota_on() and
ext4_quota_off() just update the flags as needed and avoid doing it
anywhere else... I'll have a look into it.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Add statx support

[Christoph Hellwig]

On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote:
> Return enhanced file attributes from the Ext4 filesystem.  This includes
> the following:

Just as the comment to a similar patch from Darrick for XFS:  Please add
test cases that verify this information.

Especially in the light of the races mentioned in the discussion later.

If we can't get tests for statx before 4.11 is getting close to release
I fear we'll have to revert it - untested syscalls have a tendency to
be broken and create problems for the future.

Re: [PATCH] ext4: Add statx support

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> Ultimately this needs to be addressed in ext4 more fully, but how about for
> ->getattr() just skipping the call to ext4_get_inode_flags() and instead
> populating the generic attributes like STATX_ATTR_APPEND and
> STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the
> ext4-specific flags?  Actually, it could even be done in generic_fillattr(), so
> that all filesystems benefit.

For the moment, taking Andreas's comments into account, I'll just drop the
call to ext4_get_inode_flags() and just assume ei->i_flags is correct.

I'm not sure whether to push the APPEND and IMMUTABLE attribute transfers to
the generic code - it makes sense for all filesystems that support such flags
and not for those that don't:-/  And, true, the same could be said of the
AUTOMMOUNT attribute.

David

Re: [PATCH] ext4: Add statx support

[Jan Kara]

On Sun 19-03-17 12:12:37, Jan Kara wrote:
> On Fri 17-03-17 14:19:22, Andreas Dilger wrote:
> > On Mar 16, 2017, at 11:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > > On Thu, Mar 16, 2017 at 11:35:33AM +0000, David Howells wrote:
> > >> +
> > >> +	ext4_get_inode_flags(ei);
> > >> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
> > >> +	if (flags & EXT4_APPEND_FL)
> > >> +		stat->attributes |= STATX_ATTR_APPEND;
> > >> +	if (flags & EXT4_COMPR_FL)
> > >> +		stat->attributes |= STATX_ATTR_COMPRESSED;
> > >> +	if (flags & EXT4_ENCRYPT_FL)
> > >> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
> > >> +	if (flags & EXT4_IMMUTABLE_FL)
> > >> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> > >> +	if (flags & EXT4_NODUMP_FL)
> > >> +		stat->attributes |= STATX_ATTR_NODUMP;
> > >> 
> > >> -	inode = d_inode(path->dentry);
> > >> 	generic_fillattr(inode, stat);
> > >> +	return 0;
> > >> +}
> > > 
> > > I think it's the wrong approach to be calling ext4_get_inode_flags() here to
> > > sync the generic inode flags (inode->i_flags) to the ext4-specific inode flags
> > > (ei->i_flags).  I know the GETFLAGS and FSGETXATTR ioctls do it too, but I
> > > think it's a mistake --- at least, when done so without holding the inode lock.
> > > The issue is that flag syncs can occur in both directions concurrently and
> > > cause an update to be lost.  For example, with thread 1 doing a stat() and
> > > thread 2 doing the SETFLAGS ioctl to set the APPEND flag:
> > > 
> > > Thread 1, ext4_get_inode_flags()	Thread 2, ext4_ioctl_setflags()
> > > -----------------------------------	---------------------------
> > > Read inode->i_flags; S_APPEND clear
> > > 					Set EXT4_APPEND_FL in ei->i_flags
> > > Clear EXT4_APPEND_FL in ei->i_flags
> > > 					Read ei->i_flags; EXT4_APPEND_FL clear
> > > 					Clear S_APPEND in inode->i_flags
> > > 
> > > So the flag set by SETFLAGS gets lost.  This bug probably hasn't really been
> > > noticable with GETFLAGS and FSGETXATTR since they're rarely used, but stat() on
> > > the other hand is very common, and I'm worried that with this change people
> > > would start seeing this race more often.
> > > 
> > > Ultimately this needs to be addressed in ext4 more fully, but how about for
> > > ->getattr() just skipping the call to ext4_get_inode_flags() and instead
> > > populating the generic attributes like STATX_ATTR_APPEND and
> > > STATX_ATTR_IMMUTABLE from the generic inode flags, rather than from the
> > > ext4-specific flags?  Actually, it could even be done in generic_fillattr(), so
> > > that all filesystems benefit.
> > 
> > Wouldn't it make more sense to just extract the ext4 flags directly from
> > ei->i_flags?  I think ext4_get_inode_flags() is only really useful when
> > the VFS inode flags are changed and they need to be propagated to the ext4
> > inode.
> > 
> > I guess the other more significant question is when/where are the VFS inode
> > flags changed that they need to be propagated into the ext4 disk inode?
> > The main avenue for changing these attribute flags that I know about is via
> > EXT4_IOC_SETFLAGS (FS_IOC_SETFLAGS), and there is one place that I could
> > find in fs/nsfs.c that sets S_IMMUTABLE but I don't think that propagates
> > down to an ext4 disk inode.
> 
> Yes, you seem to be right. And actually I have checked and XFS does not
> bother to copy inode->i_flags to its on-disk flags so it seems generally we
> are not expected to reflect inode->i_flags in on-disk state.
> 
> > It seems like the use of ext4_get_inode_flags() is largely superfluous.
> > The original commit ff9ddf7e847 indicates this was for quota inodes only.
> > I think it can be removed from EXT4_IOC_FSGETXATTR, and EXT4_IOC_GETFLAGS
> > and made conditional in ext4_do_update_inode():
> > 
> > #ifdef CONFIG_QUOTA
> >         for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> >                 if (unlikely(sb_dqopt(sb)->files[i] == inode))
> >                         ext4_get_inode_flags(EXT4_I(inode));
> >         }
> > #endif
> > 
> > Jan, what do you think?
> 
> So I think even better would be to have ext4_quota_on() and
> ext4_quota_off() just update the flags as needed and avoid doing it
> anywhere else... I'll have a look into it.

FYI I have just sent out patches to do this...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Add statx support

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3277 bytes --]

On Mar 31, 2017, at 9:28 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Return enhanced file attributes from the Ext4 filesystem.  This includes
> the following:
> 
> (1) The inode creation time (i_crtime) as stx_btime, setting STATX_BTIME.
> 
> (2) Certain FS_xxx_FL flags are mapped to stx_attribute flags.
> 
> This requires that all ext4 inodes have a getattr call, not just some of
> them, so to this end, split the ext4_getattr() function and only call part
> of it where appropriate.
> 
> Example output:
> 
> 	[root@andromeda ~]# touch foo
> 	[root@andromeda ~]# chattr +ai foo
> 	[root@andromeda ~]# /tmp/test-statx foo
> 	statx(foo) = 0
> 	results=fff
> 	  Size: 0               Blocks: 0          IO Block: 4096    regular file
> 	Device: 08:12           Inode: 2101950     Links: 1
> 	Access: (0644/-rw-r--r--)  Uid:     0   Gid:     0
> 	Access: 2016-02-11 17:08:29.031795451+0000
> 	Modify: 2016-02-11 17:08:29.031795451+0000
> 	Change: 2016-02-11 17:11:11.987790114+0000
> 	 Birth: 2016-02-11 17:08:29.031795451+0000
> 	Attributes: 0000000000000030 (-------- -------- -------- -------- -------- -------- -------- --ai----)
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

One minor nit below, but the current patch is OK.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4247d8d25687..5d02b922afa3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5390,11 +5390,40 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> int ext4_getattr(const struct path *path, struct kstat *stat,
> 		 u32 request_mask, unsigned int query_flags)
> {
> -	struct inode *inode;
> -	unsigned long long delalloc_blocks;
> +	struct inode *inode = d_inode(path->dentry);
> +	struct ext4_inode *raw_inode;
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	unsigned int flags;
> +
> +	if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
> +		stat->result_mask |= STATX_BTIME;
> +		stat->btime.tv_sec = ei->i_crtime.tv_sec;
> +		stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
> +	}
> +
> +	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;

Strictly speaking, since the relevant flags are being checked individually,
there is no value to masking with EXT4_FL_USER_VISIBLE.  That is only
required if you are returning the whole "flags" value directly.

Cheers, Andreas

> +	if (flags & EXT4_APPEND_FL)
> +		stat->attributes |= STATX_ATTR_APPEND;
> +	if (flags & EXT4_COMPR_FL)
> +		stat->attributes |= STATX_ATTR_COMPRESSED;
> +	if (flags & EXT4_ENCRYPT_FL)
> +		stat->attributes |= STATX_ATTR_ENCRYPTED;
> +	if (flags & EXT4_IMMUTABLE_FL)
> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> +	if (flags & EXT4_NODUMP_FL)
> +		stat->attributes |= STATX_ATTR_NODUMP;
> 
> -	inode = d_inode(path->dentry);
> 	generic_fillattr(inode, stat);
> +	return 0;
> +}
> +
> +int ext4_file_getattr(const struct path *path, struct kstat *stat,
> +		      u32 request_mask, unsigned int query_flags)
> +{
> +	struct inode *inode = d_inode(path->dentry);
> +	u64 delalloc_blocks;
> +
> +	ext4_getattr(path, stat, request_mask, query_flags);
> 
> 	/*
> 	 * If there is inline data in the inode, the inode will normally not






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH] ext4: Add statx support

[David Howells]

Return enhanced file attributes from the Ext4 filesystem.  This includes
the following:

 (1) The inode creation time (i_crtime) as stx_btime, setting STATX_BTIME.

 (2) Certain FS_xxx_FL flags are mapped to stx_attribute flags.

This requires that all ext4 inodes have a getattr call, not just some of
them, so to this end, split the ext4_getattr() function and only call part
of it where appropriate.

Example output:

	[root@andromeda ~]# touch foo
	[root@andromeda ~]# chattr +ai foo
	[root@andromeda ~]# /tmp/test-statx foo
	statx(foo) = 0
	results=fff
	  Size: 0               Blocks: 0          IO Block: 4096    regular file
	Device: 08:12           Inode: 2101950     Links: 1
	Access: (0644/-rw-r--r--)  Uid:     0   Gid:     0
	Access: 2016-02-11 17:08:29.031795451+0000
	Modify: 2016-02-11 17:08:29.031795451+0000
	Change: 2016-02-11 17:11:11.987790114+0000
	 Birth: 2016-02-11 17:08:29.031795451+0000
	Attributes: 0000000000000030 (-------- -------- -------- -------- -------- -------- -------- --ai----)

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/ext4/ext4.h    |    1 +
 fs/ext4/file.c    |    2 +-
 fs/ext4/inode.c   |   35 ++++++++++++++++++++++++++++++++---
 fs/ext4/namei.c   |    2 ++
 fs/ext4/symlink.c |    3 +++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f493af666591..fb69ee2388db 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2466,6 +2466,7 @@ extern int  ext4_setattr(struct dentry *, struct iattr *);
 extern int  ext4_getattr(const struct path *, struct kstat *, u32, unsigned int);
 extern void ext4_evict_inode(struct inode *);
 extern void ext4_clear_inode(struct inode *);
+extern int  ext4_file_getattr(const struct path *, struct kstat *, u32, unsigned int);
 extern int  ext4_sync_inode(handle_t *, struct inode *);
 extern void ext4_dirty_inode(struct inode *, int);
 extern int ext4_change_inode_journal_flag(struct inode *, int);
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8210c1f43556..cefa9835f275 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -744,7 +744,7 @@ const struct file_operations ext4_file_operations = {
 
 const struct inode_operations ext4_file_inode_operations = {
 	.setattr	= ext4_setattr,
-	.getattr	= ext4_getattr,
+	.getattr	= ext4_file_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4247d8d25687..5d02b922afa3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5390,11 +5390,40 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 int ext4_getattr(const struct path *path, struct kstat *stat,
 		 u32 request_mask, unsigned int query_flags)
 {
-	struct inode *inode;
-	unsigned long long delalloc_blocks;
+	struct inode *inode = d_inode(path->dentry);
+	struct ext4_inode *raw_inode;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	unsigned int flags;
+
+	if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
+		stat->result_mask |= STATX_BTIME;
+		stat->btime.tv_sec = ei->i_crtime.tv_sec;
+		stat->btime.tv_nsec = ei->i_crtime.tv_nsec;
+	}
+
+	flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
+	if (flags & EXT4_APPEND_FL)
+		stat->attributes |= STATX_ATTR_APPEND;
+	if (flags & EXT4_COMPR_FL)
+		stat->attributes |= STATX_ATTR_COMPRESSED;
+	if (flags & EXT4_ENCRYPT_FL)
+		stat->attributes |= STATX_ATTR_ENCRYPTED;
+	if (flags & EXT4_IMMUTABLE_FL)
+		stat->attributes |= STATX_ATTR_IMMUTABLE;
+	if (flags & EXT4_NODUMP_FL)
+		stat->attributes |= STATX_ATTR_NODUMP;
 
-	inode = d_inode(path->dentry);
 	generic_fillattr(inode, stat);
+	return 0;
+}
+
+int ext4_file_getattr(const struct path *path, struct kstat *stat,
+		      u32 request_mask, unsigned int query_flags)
+{
+	struct inode *inode = d_inode(path->dentry);
+	u64 delalloc_blocks;
+
+	ext4_getattr(path, stat, request_mask, query_flags);
 
 	/*
 	 * If there is inline data in the inode, the inode will normally not
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 6ad612c576fc..07e5e1405771 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3912,6 +3912,7 @@ const struct inode_operations ext4_dir_inode_operations = {
 	.tmpfile	= ext4_tmpfile,
 	.rename		= ext4_rename2,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
@@ -3920,6 +3921,7 @@ const struct inode_operations ext4_dir_inode_operations = {
 
 const struct inode_operations ext4_special_inode_operations = {
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 	.get_acl	= ext4_get_acl,
 	.set_acl	= ext4_set_acl,
diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
index 73b184d161fc..5c8fc53cb0e5 100644
--- a/fs/ext4/symlink.c
+++ b/fs/ext4/symlink.c
@@ -85,17 +85,20 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
 const struct inode_operations ext4_encrypted_symlink_inode_operations = {
 	.get_link	= ext4_encrypted_get_link,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 };
 
 const struct inode_operations ext4_symlink_inode_operations = {
 	.get_link	= page_get_link,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 };
 
 const struct inode_operations ext4_fast_symlink_inode_operations = {
 	.get_link	= simple_get_link,
 	.setattr	= ext4_setattr,
+	.getattr	= ext4_getattr,
 	.listxattr	= ext4_listxattr,
 };