[PATCH RFC 0/8] xfs: introduce inode data inline feature

[PATCH RFC 0/8] xfs: introduce inode data inline feature

[Shan Hai]

This series implements xfs inode data inlining feature.

Refered below link during development:
https://marc.info/?l=linux-xfs&m=120493585731509&w=2


How it works:
- the data inlining happens at:
  write_iter: for DIO/DAX write
  writeback: for buffered write
- extents to local format conversion is done in writeback but not in write_iter
- local to extents format conversion is done in the write_iter and writeback
- log the whole inode (core/data) on extents to local conversion
- add a new mkfs.xfs option to enable data inline feature

How to test:
mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdX
mount /dev/sdX mnt_pnt

Test results:
mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdb
mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdd
mount /dev/sdb /mnt/sdb
mount /dev/sdd /mnt/sdd

Copy the linux v4.18-rc3 source code to /mnt/sdb and /mnt/sdd respectively:
Filesystem                        Size  Used Avail Use% Mounted on
/dev/sdb                           10G  2.4G  7.7G  24% /mnt/sdb
/dev/sdd                           10G  3.1G  6.9G  31% /mnt/sdd

mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdc
mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sde
mount /dev/sdc /mnt/sdc
mount /dev/sde /mnt/sde

Build the v4.18-rc3 kernel on /dev/sdc and /dev/sde respectively:
make x86_64_defconfig

Filesystem                        Size  Used Avail Use% Mounted on
/dev/sdc                           10G  689M  9.4G   7% /mnt/sdc
/dev/sde                           10G  694M  9.4G   7% /mnt/sde

Kernel part:
0001-xfs-introduce-inline-data-superblock-feature-bit.patch
0002-xfs-introduce-extents-to-local-conversion-helper.patch
0003-xfs-convert-inode-from-extents-to-local-format.patch
0004-xfs-implement-inline-data-read-write-code.patch
0005-xfs-consider-the-local-format-inode-in-misc-operatio.patch
0006-xfs-fix-imbalanced-locking.patch
0007-xfs-return-non-zero-blocks-for-inline-data.patch
0008-xfs-skip-local-format-inode-for-reflinking.patch

 fs/xfs/libxfs/xfs_bmap.c      |  78 +++++++++++++++++++++++++++++++-----
 fs/xfs/libxfs/xfs_bmap.h      |   4 ++
 fs/xfs/libxfs/xfs_format.h    |  10 ++++-
 fs/xfs/libxfs/xfs_fs.h        |   1 +
 fs/xfs/libxfs/xfs_inode_buf.c |   6 ---
 fs/xfs/libxfs/xfs_sb.c        |   2 +
 fs/xfs/scrub/inode.c          |   2 +-
 fs/xfs/xfs_aops.c             |  60 ++++++++++++++++++++++++++++
 fs/xfs/xfs_bmap_util.c        |  11 +++++-
 fs/xfs/xfs_file.c             | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_inode.c            |  20 +++++++---
 fs/xfs/xfs_inode_item.c       |   5 ++-
 fs/xfs/xfs_ioctl.c            |   5 ++-
 fs/xfs/xfs_iomap.c            |   5 ++-
 fs/xfs/xfs_iops.c             |   8 +++-
 fs/xfs/xfs_log_recover.c      |   3 +-
 fs/xfs/xfs_reflink.c          |   6 +++
 17 files changed, 437 insertions(+), 33 deletions(-)

xfsprogs part:
0001-xfsprogs-add-inode-inline-data-support.patch 

 growfs/xfs_growfs.c | 14 +++++++++-----
 libxfs/xfs_format.h |  4 +++-
 libxfs/xfs_fs.h     |  1 +
 mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
 4 files changed, 30 insertions(+), 9 deletions(-)


Thanks
Shan Hai

[PATCH RFC 1/8] xfs: introduce inline data superblock feature bit

[Shan Hai]

The data inlining is an on-disk format change, so introduce a new
superblock feature bit to work with the mkfs tool to handle the
inline data, the feature bit is set at mkfs time.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/libxfs/xfs_format.h | 10 +++++++++-
 fs/xfs/libxfs/xfs_fs.h     |  1 +
 fs/xfs/libxfs/xfs_sb.c     |  2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 059bc44c27e8..6066ba7fdaf7 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -469,10 +469,12 @@ xfs_sb_has_ro_compat_feature(
 #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
 #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
 #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
+#define XFS_SB_FEAT_INCOMPAT_INLINEDATA (1 << 3)	/* inline inode data */
 #define XFS_SB_FEAT_INCOMPAT_ALL \
 		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
 		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
-		 XFS_SB_FEAT_INCOMPAT_META_UUID)
+		 XFS_SB_FEAT_INCOMPAT_META_UUID|\
+		 XFS_SB_FEAT_INCOMPAT_INLINEDATA)
 
 #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
 static inline bool
@@ -550,6 +552,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
 		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
 }
 
+static inline bool xfs_sb_version_hasinlinedata(struct xfs_sb *sbp)
+{
+	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) &&
+		(sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_INLINEDATA);
+}
+
 /*
  * end of superblock version macros
  */
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index f3aa59302fef..ee1f7621c0e4 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -210,6 +210,7 @@ typedef struct xfs_fsop_resblks {
 #define XFS_FSOP_GEOM_FLAGS_SPINODES	0x40000	/* sparse inode chunks	*/
 #define XFS_FSOP_GEOM_FLAGS_RMAPBT	0x80000	/* reverse mapping btree */
 #define XFS_FSOP_GEOM_FLAGS_REFLINK	0x100000 /* files can share blocks */
+#define XFS_FSOP_GEOM_FLAGS_INLINEDATA	0x200000 /* inline data into inode */
 
 /*
  * Minimum and maximum sizes need for growth checks.
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 350119eeaecb..4002ed105b6b 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1068,6 +1068,8 @@ xfs_fs_geometry(
 		geo->logsectsize = sbp->sb_logsectsize;
 	else
 		geo->logsectsize = BBSIZE;
+	if (xfs_sb_version_hasinlinedata(sbp))
+		geo->flags |= XFS_FSOP_GEOM_FLAGS_INLINEDATA;
 	geo->rtsectsize = sbp->sb_blocksize;
 	geo->dirblocksize = xfs_dir2_dirblock_bytes(sbp);
 
-- 
2.11.0

Re: [PATCH RFC 1/8] xfs: introduce inline data superblock feature bit

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:22AM +0800, Shan Hai wrote:
> The data inlining is an on-disk format change, so introduce a new
> superblock feature bit to work with the mkfs tool to handle the
> inline data, the feature bit is set at mkfs time.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h | 10 +++++++++-
>  fs/xfs/libxfs/xfs_fs.h     |  1 +
>  fs/xfs/libxfs/xfs_sb.c     |  2 ++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 059bc44c27e8..6066ba7fdaf7 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -469,10 +469,12 @@ xfs_sb_has_ro_compat_feature(
>  #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
>  #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
>  #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
> +#define XFS_SB_FEAT_INCOMPAT_INLINEDATA (1 << 3)	/* inline inode data */
>  #define XFS_SB_FEAT_INCOMPAT_ALL \
>  		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
>  		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
> -		 XFS_SB_FEAT_INCOMPAT_META_UUID)
> +		 XFS_SB_FEAT_INCOMPAT_META_UUID|\
> +		 XFS_SB_FEAT_INCOMPAT_INLINEDATA)

Uh... don't go enabling this feature in the _ALL flags until you've
finished putting all the code in...

>  
>  #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
>  static inline bool
> @@ -550,6 +552,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
>  		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
>  }
>  
> +static inline bool xfs_sb_version_hasinlinedata(struct xfs_sb *sbp)
> +{
> +	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) &&
> +		(sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_INLINEDATA);
> +}
> +
>  /*
>   * end of superblock version macros
>   */
> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> index f3aa59302fef..ee1f7621c0e4 100644
> --- a/fs/xfs/libxfs/xfs_fs.h
> +++ b/fs/xfs/libxfs/xfs_fs.h
> @@ -210,6 +210,7 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_FSOP_GEOM_FLAGS_SPINODES	0x40000	/* sparse inode chunks	*/
>  #define XFS_FSOP_GEOM_FLAGS_RMAPBT	0x80000	/* reverse mapping btree */
>  #define XFS_FSOP_GEOM_FLAGS_REFLINK	0x100000 /* files can share blocks */
> +#define XFS_FSOP_GEOM_FLAGS_INLINEDATA	0x200000 /* inline data into inode */
>  
>  /*
>   * Minimum and maximum sizes need for growth checks.
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 350119eeaecb..4002ed105b6b 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -1068,6 +1068,8 @@ xfs_fs_geometry(
>  		geo->logsectsize = sbp->sb_logsectsize;
>  	else
>  		geo->logsectsize = BBSIZE;
> +	if (xfs_sb_version_hasinlinedata(sbp))
> +		geo->flags |= XFS_FSOP_GEOM_FLAGS_INLINEDATA;
>  	geo->rtsectsize = sbp->sb_blocksize;
>  	geo->dirblocksize = xfs_dir2_dirblock_bytes(sbp);
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/8] xfs: introduce inline data superblock feature bit

[Shan Hai]

On 2018年07月06日 11:34, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:22AM +0800, Shan Hai wrote:
>> The data inlining is an on-disk format change, so introduce a new
>> superblock feature bit to work with the mkfs tool to handle the
>> inline data, the feature bit is set at mkfs time.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_format.h | 10 +++++++++-
>>   fs/xfs/libxfs/xfs_fs.h     |  1 +
>>   fs/xfs/libxfs/xfs_sb.c     |  2 ++
>>   3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 059bc44c27e8..6066ba7fdaf7 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -469,10 +469,12 @@ xfs_sb_has_ro_compat_feature(
>>   #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
>>   #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
>>   #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
>> +#define XFS_SB_FEAT_INCOMPAT_INLINEDATA (1 << 3)	/* inline inode data */
>>   #define XFS_SB_FEAT_INCOMPAT_ALL \
>>   		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
>>   		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
>> -		 XFS_SB_FEAT_INCOMPAT_META_UUID)
>> +		 XFS_SB_FEAT_INCOMPAT_META_UUID|\
>> +		 XFS_SB_FEAT_INCOMPAT_INLINEDATA)
> Uh... don't go enabling this feature in the _ALL flags until you've
> finished putting all the code in...
>

Agreed, will fix it, thanks for the suggestion.

Thanks
Shan Hai
>>   
>>   #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
>>   static inline bool
>> @@ -550,6 +552,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
>>   		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
>>   }
>>   
>> +static inline bool xfs_sb_version_hasinlinedata(struct xfs_sb *sbp)
>> +{
>> +	return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) &&
>> +		(sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_INLINEDATA);
>> +}
>> +
>>   /*
>>    * end of superblock version macros
>>    */
>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> index f3aa59302fef..ee1f7621c0e4 100644
>> --- a/fs/xfs/libxfs/xfs_fs.h
>> +++ b/fs/xfs/libxfs/xfs_fs.h
>> @@ -210,6 +210,7 @@ typedef struct xfs_fsop_resblks {
>>   #define XFS_FSOP_GEOM_FLAGS_SPINODES	0x40000	/* sparse inode chunks	*/
>>   #define XFS_FSOP_GEOM_FLAGS_RMAPBT	0x80000	/* reverse mapping btree */
>>   #define XFS_FSOP_GEOM_FLAGS_REFLINK	0x100000 /* files can share blocks */
>> +#define XFS_FSOP_GEOM_FLAGS_INLINEDATA	0x200000 /* inline data into inode */
>>   
>>   /*
>>    * Minimum and maximum sizes need for growth checks.
>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>> index 350119eeaecb..4002ed105b6b 100644
>> --- a/fs/xfs/libxfs/xfs_sb.c
>> +++ b/fs/xfs/libxfs/xfs_sb.c
>> @@ -1068,6 +1068,8 @@ xfs_fs_geometry(
>>   		geo->logsectsize = sbp->sb_logsectsize;
>>   	else
>>   		geo->logsectsize = BBSIZE;
>> +	if (xfs_sb_version_hasinlinedata(sbp))
>> +		geo->flags |= XFS_FSOP_GEOM_FLAGS_INLINEDATA;
>>   	geo->rtsectsize = sbp->sb_blocksize;
>>   	geo->dirblocksize = xfs_dir2_dirblock_bytes(sbp);
>>   
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 2/8] xfs: introduce extents to local conversion helper

[Shan Hai]

Delete the extents from xfs inode, copy the data into the data fork,
convert the inode from extents to local format and specify log the
core and data fork of the inode.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/libxfs/xfs_bmap.h |  4 ++++
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7205268b30bc..bea6dc254a7d 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2000-2006 Silicon Graphics, Inc.
+ * Copyright (c) 2018 Oracle.
  * All Rights Reserved.
  */
 #include "xfs.h"
@@ -213,7 +214,7 @@ xfs_default_attroffset(
  * attribute fork from local to extent format - we reset it where
  * possible to make space available for inline data fork extents.
  */
-STATIC void
+void
 xfs_bmap_forkoff_reset(
 	xfs_inode_t	*ip,
 	int		whichfork)
@@ -5141,6 +5142,63 @@ xfs_bmap_del_extent_real(
 }
 
 /*
+ * Convert an inode from extents to the local format.
+ * Free all the extents of the inode and reset it to the local
+ * format. Copy the contents of the inode's blocks to the inode's
+ * literal area.
+ */
+int
+xfs_bmap_extents_to_local(
+	xfs_trans_t		*tp,
+	xfs_inode_t		*ip,
+	struct xfs_defer_ops	*dfops,
+	int			*logflagsp,
+	int			whichfork,
+	struct page		*page)
+{
+	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
+	xfs_fileoff_t		isize = i_size_read(VFS_I(ip));
+	struct xfs_bmbt_irec	got, del;
+	struct xfs_iext_cursor	icur;
+	int			error = 0;
+	int			tmp_logflags;
+	char			*kaddr = NULL;
+
+	ASSERT(whichfork != XFS_COW_FORK);
+	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS);
+
+	if (xfs_iext_count(ifp) == 0)
+		goto init_local_fork;
+
+	for_each_xfs_iext(ifp, &icur, &got) {
+		del = got;
+		if (isnullstartblock(got.br_startblock)) {
+			error = xfs_bmap_del_extent_delay(ip,
+					whichfork, &icur, &got, &del);
+			if (error)
+				goto out;
+		} else {
+			error = xfs_bmap_del_extent_real(ip, tp, &icur, dfops,
+					NULL, &del, &tmp_logflags, whichfork, 0);
+			if (error)
+				goto out;
+			*logflagsp |= tmp_logflags;
+		}
+	}
+init_local_fork:
+	kaddr = kmap_atomic(page);
+	xfs_init_local_fork(ip, whichfork, kaddr, isize);
+	kunmap_atomic(kaddr);
+	ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
+	XFS_IFORK_NEXT_SET(ip, whichfork, 0);
+	ip->i_d.di_size = isize;
+	*logflagsp |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
+out:
+	return error;
+}
+
+
+/*
  * Unmap (remove) blocks from a file.
  * If nexts is nonzero then the number of extents to remove is limited to
  * that value.  If not all extents in the block range can be removed then
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 9b49ddf99c41..22cd2642f1cd 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -271,6 +271,10 @@ int	xfs_bmap_map_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
 		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
 int	xfs_bmap_unmap_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
 		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
+int	xfs_bmap_extents_to_local(struct xfs_trans *tp, struct xfs_inode *ip,
+		struct xfs_defer_ops *dfops, int *flags, int whichfork,
+		struct page *page);
+void	xfs_bmap_forkoff_reset(struct xfs_inode *ip, int whichfork);
 
 static inline int xfs_bmap_fork_to_state(int whichfork)
 {
-- 
2.11.0

Re: [PATCH RFC 2/8] xfs: introduce extents to local conversion helper

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:23AM +0800, Shan Hai wrote:
> Delete the extents from xfs inode, copy the data into the data fork,
> convert the inode from extents to local format and specify log the
> core and data fork of the inode.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/libxfs/xfs_bmap.h |  4 ++++
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 7205268b30bc..bea6dc254a7d 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2000-2006 Silicon Graphics, Inc.
> + * Copyright (c) 2018 Oracle.
>   * All Rights Reserved.
>   */
>  #include "xfs.h"
> @@ -213,7 +214,7 @@ xfs_default_attroffset(
>   * attribute fork from local to extent format - we reset it where
>   * possible to make space available for inline data fork extents.
>   */
> -STATIC void
> +void
>  xfs_bmap_forkoff_reset(

Unrelated change in this patch?

Also, is it safe to reset forkoff when converting the data fork to
extents?  Does doing so mes up the attr fork?

>  	xfs_inode_t	*ip,
>  	int		whichfork)
> @@ -5141,6 +5142,63 @@ xfs_bmap_del_extent_real(
>  }
>  
>  /*
> + * Convert an inode from extents to the local format.
> + * Free all the extents of the inode and reset it to the local
> + * format. Copy the contents of the inode's blocks to the inode's
> + * literal area.
> + */
> +int
> +xfs_bmap_extents_to_local(
> +	xfs_trans_t		*tp,
> +	xfs_inode_t		*ip,

No typedefs, please.

	struct xfs_inode	*ip,

> +	struct xfs_defer_ops	*dfops,
> +	int			*logflagsp,
> +	int			whichfork,
> +	struct page		*page)
> +{
> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	xfs_fileoff_t		isize = i_size_read(VFS_I(ip));
> +	struct xfs_bmbt_irec	got, del;
> +	struct xfs_iext_cursor	icur;
> +	int			error = 0;
> +	int			tmp_logflags;
> +	char			*kaddr = NULL;
> +
> +	ASSERT(whichfork != XFS_COW_FORK);
> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS);
> +
> +	if (xfs_iext_count(ifp) == 0)
> +		goto init_local_fork;
> +
> +	for_each_xfs_iext(ifp, &icur, &got) {
> +		del = got;
> +		if (isnullstartblock(got.br_startblock)) {
> +			error = xfs_bmap_del_extent_delay(ip,
> +					whichfork, &icur, &got, &del);
> +			if (error)
> +				goto out;
> +		} else {
> +			error = xfs_bmap_del_extent_real(ip, tp, &icur, dfops,
> +					NULL, &del, &tmp_logflags, whichfork, 0);
> +			if (error)
> +				goto out;
> +			*logflagsp |= tmp_logflags;
> +		}
> +	}
> +init_local_fork:
> +	kaddr = kmap_atomic(page);
> +	xfs_init_local_fork(ip, whichfork, kaddr, isize);
> +	kunmap_atomic(kaddr);
> +	ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
> +	XFS_IFORK_NEXT_SET(ip, whichfork, 0);
> +	ip->i_d.di_size = isize;
> +	*logflagsp |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
> +out:
> +	return error;
> +}
> +
> +
> +/*
>   * Unmap (remove) blocks from a file.
>   * If nexts is nonzero then the number of extents to remove is limited to
>   * that value.  If not all extents in the block range can be removed then
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 9b49ddf99c41..22cd2642f1cd 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -271,6 +271,10 @@ int	xfs_bmap_map_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
>  		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
>  int	xfs_bmap_unmap_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
>  		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
> +int	xfs_bmap_extents_to_local(struct xfs_trans *tp, struct xfs_inode *ip,
> +		struct xfs_defer_ops *dfops, int *flags, int whichfork,
> +		struct page *page);
> +void	xfs_bmap_forkoff_reset(struct xfs_inode *ip, int whichfork);
>  
>  static inline int xfs_bmap_fork_to_state(int whichfork)
>  {
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/8] xfs: introduce extents to local conversion helper

[Shan Hai]

On 2018年07月06日 11:45, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:23AM +0800, Shan Hai wrote:
>> Delete the extents from xfs inode, copy the data into the data fork,
>> convert the inode from extents to local format and specify log the
>> core and data fork of the inode.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_bmap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   fs/xfs/libxfs/xfs_bmap.h |  4 ++++
>>   2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 7205268b30bc..bea6dc254a7d 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0
>>   /*
>>    * Copyright (c) 2000-2006 Silicon Graphics, Inc.
>> + * Copyright (c) 2018 Oracle.
>>    * All Rights Reserved.
>>    */
>>   #include "xfs.h"
>> @@ -213,7 +214,7 @@ xfs_default_attroffset(
>>    * attribute fork from local to extent format - we reset it where
>>    * possible to make space available for inline data fork extents.
>>    */
>> -STATIC void
>> +void
>>   xfs_bmap_forkoff_reset(
> Unrelated change in this patch?

Need this in the 3rd patch to convert the inode from extents to local, 
so export it,
because I thought that the conversion function should reside in the 
xfs_file.c instead
of xfs_bmap.c.

> Also, is it safe to reset forkoff when converting the data fork to
> extents?  Does doing so mes up the attr fork?
>

The attr fork should be aware the forkoff changes, I will double check 
this anyway.

Thanks
Shan Hai
>>   	xfs_inode_t	*ip,
>>   	int		whichfork)
>> @@ -5141,6 +5142,63 @@ xfs_bmap_del_extent_real(
>>   }
>>   
>>   /*
>> + * Convert an inode from extents to the local format.
>> + * Free all the extents of the inode and reset it to the local
>> + * format. Copy the contents of the inode's blocks to the inode's
>> + * literal area.
>> + */
>> +int
>> +xfs_bmap_extents_to_local(
>> +	xfs_trans_t		*tp,
>> +	xfs_inode_t		*ip,
> No typedefs, please.
>
> 	struct xfs_inode	*ip,

OK, I will fix it.

Thanks
Shan Hai
>> +	struct xfs_defer_ops	*dfops,
>> +	int			*logflagsp,
>> +	int			whichfork,
>> +	struct page		*page)
>> +{
>> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
>> +	xfs_fileoff_t		isize = i_size_read(VFS_I(ip));
>> +	struct xfs_bmbt_irec	got, del;
>> +	struct xfs_iext_cursor	icur;
>> +	int			error = 0;
>> +	int			tmp_logflags;
>> +	char			*kaddr = NULL;
>> +
>> +	ASSERT(whichfork != XFS_COW_FORK);
>> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS);
>> +
>> +	if (xfs_iext_count(ifp) == 0)
>> +		goto init_local_fork;
>> +
>> +	for_each_xfs_iext(ifp, &icur, &got) {
>> +		del = got;
>> +		if (isnullstartblock(got.br_startblock)) {
>> +			error = xfs_bmap_del_extent_delay(ip,
>> +					whichfork, &icur, &got, &del);
>> +			if (error)
>> +				goto out;
>> +		} else {
>> +			error = xfs_bmap_del_extent_real(ip, tp, &icur, dfops,
>> +					NULL, &del, &tmp_logflags, whichfork, 0);
>> +			if (error)
>> +				goto out;
>> +			*logflagsp |= tmp_logflags;
>> +		}
>> +	}
>> +init_local_fork:
>> +	kaddr = kmap_atomic(page);
>> +	xfs_init_local_fork(ip, whichfork, kaddr, isize);
>> +	kunmap_atomic(kaddr);
>> +	ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
>> +	XFS_IFORK_NEXT_SET(ip, whichfork, 0);
>> +	ip->i_d.di_size = isize;
>> +	*logflagsp |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
>> +out:
>> +	return error;
>> +}
>> +
>> +
>> +/*
>>    * Unmap (remove) blocks from a file.
>>    * If nexts is nonzero then the number of extents to remove is limited to
>>    * that value.  If not all extents in the block range can be removed then
>> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
>> index 9b49ddf99c41..22cd2642f1cd 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.h
>> +++ b/fs/xfs/libxfs/xfs_bmap.h
>> @@ -271,6 +271,10 @@ int	xfs_bmap_map_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
>>   		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
>>   int	xfs_bmap_unmap_extent(struct xfs_mount *mp, struct xfs_defer_ops *dfops,
>>   		struct xfs_inode *ip, struct xfs_bmbt_irec *imap);
>> +int	xfs_bmap_extents_to_local(struct xfs_trans *tp, struct xfs_inode *ip,
>> +		struct xfs_defer_ops *dfops, int *flags, int whichfork,
>> +		struct page *page);
>> +void	xfs_bmap_forkoff_reset(struct xfs_inode *ip, int whichfork);
>>   
>>   static inline int xfs_bmap_fork_to_state(int whichfork)
>>   {
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/8] xfs: introduce extents to local conversion helper

[Christoph Hellwig]

>  /*
> + * Convert an inode from extents to the local format.
> + * Free all the extents of the inode and reset it to the local
> + * format. Copy the contents of the inode's blocks to the inode's
> + * literal area.
> + */

Please use up all 80 chars for your comments (and code)

> +int
> +xfs_bmap_extents_to_local(
> +	xfs_trans_t		*tp,
> +	xfs_inode_t		*ip,

Please use the struct types instead of the typedefs wherever possible.

> +	if (xfs_iext_count(ifp) == 0)
> +		goto init_local_fork;
> +
> +	for_each_xfs_iext(ifp, &icur, &got) {

for_each_xfs_iext should do the right thing for an empty fork.

> +out:
> +	return error;

Just return errors directly instead of going to a label that returns
the error.

> +}
> +
> +
> +/*

One blank line after a function body is enough.

Re: [PATCH RFC 2/8] xfs: introduce extents to local conversion helper

[Shan Hai]

On 2018年07月08日 23:42, Christoph Hellwig wrote:
>>   /*
>> + * Convert an inode from extents to the local format.
>> + * Free all the extents of the inode and reset it to the local
>> + * format. Copy the contents of the inode's blocks to the inode's
>> + * literal area.
>> + */
> Please use up all 80 chars for your comments (and code)
>
>> +int
>> +xfs_bmap_extents_to_local(
>> +	xfs_trans_t		*tp,
>> +	xfs_inode_t		*ip,
> Please use the struct types instead of the typedefs wherever possible.
>
>> +	if (xfs_iext_count(ifp) == 0)
>> +		goto init_local_fork;
>> +
>> +	for_each_xfs_iext(ifp, &icur, &got) {
> for_each_xfs_iext should do the right thing for an empty fork.
>
>> +out:
>> +	return error;
> Just return errors directly instead of going to a label that returns
> the error.
>
>> +}
>> +
>> +
>> +/*
> One blank line after a function body is enough.
>

OK, I will fix all of the above, thanks for the review.

Thanks
Shan Hai

[PATCH RFC 3/8] xfs: convert inode from extents to local format

[Shan Hai]

Introduce a function to convert an inode from extents to local
format. The conversion happens at writeback time, this avoids
interfering with the delayed allocation and page cache vs
xfs_buf operations, the will be inlined data is directly got
from the page which was read and modified by the iomap write actor.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/xfs_aops.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 8eb3ba3d4d00..ac5a7695f363 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -9,6 +9,7 @@
 #include "xfs_log_format.h"
 #include "xfs_trans_resv.h"
 #include "xfs_mount.h"
+#include "xfs_defer.h"
 #include "xfs_inode.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
@@ -887,6 +888,55 @@ xfs_map_cow(
 	return 0;
 }
 
+static int
+xfs_inode_extents_to_local(
+	xfs_inode_t		*ip,
+	int			whichfork,
+	struct page		*page)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
+	xfs_fsblock_t		first_block;
+	int			logflags;
+	int			error = 0;
+
+	if (i_size_read(VFS_I(ip)) > XFS_IFORK_DSIZE(ip))
+		return 0;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+
+	xfs_defer_init(&dfops, &first_block);
+
+	logflags = 0;
+	error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
+			XFS_DATA_FORK, page);
+	if (error)
+		goto trans_cancel;
+
+	xfs_trans_log_inode(tp, ip, logflags);
+	error = xfs_defer_finish(&tp, &dfops);
+	if (error)
+		goto trans_cancel;
+	error = xfs_trans_commit(tp);
+	if (error)
+		goto error0;
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
+	return 0;
+
+trans_cancel:
+	xfs_defer_cancel(&dfops);
+	xfs_trans_cancel(tp);
+error0:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+}
 /*
  * We implement an immediate ioend submission policy here to avoid needing to
  * chain multiple ioends and hence nest mempool allocations which can violate
@@ -920,6 +970,8 @@ xfs_writepage_map(
 	int			count = 0;
 	int			uptodate = 1;
 	unsigned int		new_type;
+	xfs_inode_t		*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
 
 	bh = head = page_buffers(page);
 	offset = page_offset(page);
@@ -1044,6 +1096,14 @@ xfs_writepage_map(
 		end_page_writeback(page);
 	}
 
+	if (!error) {
+		if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+				i_size_read(inode) <= XFS_IFORK_DSIZE(ip)) {
+			error = xfs_inode_extents_to_local(ip, XFS_DATA_FORK,
+								page);
+		}
+	}
+
 	mapping_set_error(page->mapping, error);
 	return error;
 }
-- 
2.11.0

Re: [PATCH RFC 3/8] xfs: convert inode from extents to local format

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:24AM +0800, Shan Hai wrote:
> Introduce a function to convert an inode from extents to local
> format. The conversion happens at writeback time, this avoids
> interfering with the delayed allocation and page cache vs
> xfs_buf operations, the will be inlined data is directly got
> from the page which was read and modified by the iomap write actor.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_aops.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 8eb3ba3d4d00..ac5a7695f363 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -9,6 +9,7 @@
>  #include "xfs_log_format.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
> +#include "xfs_defer.h"
>  #include "xfs_inode.h"
>  #include "xfs_trans.h"
>  #include "xfs_inode_item.h"
> @@ -887,6 +888,55 @@ xfs_map_cow(
>  	return 0;
>  }
>  
> +static int
> +xfs_inode_extents_to_local(
> +	xfs_inode_t		*ip,
> +	int			whichfork,
> +	struct page		*page)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	struct xfs_defer_ops	dfops;
> +	xfs_fsblock_t		first_block;
> +	int			logflags;
> +	int			error = 0;
> +
> +	if (i_size_read(VFS_I(ip)) > XFS_IFORK_DSIZE(ip))
> +		return 0;

If it's too big to fit in local format, then isn't this an error?

--D

> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	xfs_defer_init(&dfops, &first_block);
> +
> +	logflags = 0;
> +	error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
> +			XFS_DATA_FORK, page);
> +	if (error)
> +		goto trans_cancel;
> +
> +	xfs_trans_log_inode(tp, ip, logflags);
> +	error = xfs_defer_finish(&tp, &dfops);
> +	if (error)
> +		goto trans_cancel;
> +	error = xfs_trans_commit(tp);
> +	if (error)
> +		goto error0;
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +
> +	return 0;
> +
> +trans_cancel:
> +	xfs_defer_cancel(&dfops);
> +	xfs_trans_cancel(tp);
> +error0:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return error;
> +}
>  /*
>   * We implement an immediate ioend submission policy here to avoid needing to
>   * chain multiple ioends and hence nest mempool allocations which can violate
> @@ -920,6 +970,8 @@ xfs_writepage_map(
>  	int			count = 0;
>  	int			uptodate = 1;
>  	unsigned int		new_type;
> +	xfs_inode_t		*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
>  
>  	bh = head = page_buffers(page);
>  	offset = page_offset(page);
> @@ -1044,6 +1096,14 @@ xfs_writepage_map(
>  		end_page_writeback(page);
>  	}
>  
> +	if (!error) {
> +		if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +				i_size_read(inode) <= XFS_IFORK_DSIZE(ip)) {
> +			error = xfs_inode_extents_to_local(ip, XFS_DATA_FORK,
> +								page);
> +		}
> +	}
> +
>  	mapping_set_error(page->mapping, error);
>  	return error;
>  }
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 3/8] xfs: convert inode from extents to local format

[Shan Hai]

On 2018年07月06日 11:47, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:24AM +0800, Shan Hai wrote:
>> Introduce a function to convert an inode from extents to local
>> format. The conversion happens at writeback time, this avoids
>> interfering with the delayed allocation and page cache vs
>> xfs_buf operations, the will be inlined data is directly got
>> from the page which was read and modified by the iomap write actor.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_aops.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
>> index 8eb3ba3d4d00..ac5a7695f363 100644
>> --- a/fs/xfs/xfs_aops.c
>> +++ b/fs/xfs/xfs_aops.c
>> @@ -9,6 +9,7 @@
>>   #include "xfs_log_format.h"
>>   #include "xfs_trans_resv.h"
>>   #include "xfs_mount.h"
>> +#include "xfs_defer.h"
>>   #include "xfs_inode.h"
>>   #include "xfs_trans.h"
>>   #include "xfs_inode_item.h"
>> @@ -887,6 +888,55 @@ xfs_map_cow(
>>   	return 0;
>>   }
>>   
>> +static int
>> +xfs_inode_extents_to_local(
>> +	xfs_inode_t		*ip,
>> +	int			whichfork,
>> +	struct page		*page)
>> +{
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	struct xfs_trans	*tp;
>> +	struct xfs_defer_ops	dfops;
>> +	xfs_fsblock_t		first_block;
>> +	int			logflags;
>> +	int			error = 0;
>> +
>> +	if (i_size_read(VFS_I(ip)) > XFS_IFORK_DSIZE(ip))
>> +		return 0;
> If it's too big to fit in local format, then isn't this an error?

No, it's not, because the written data travels to writeback means that 
the inode is
still in extents format since the higher level write_iter ends the write 
process by
copying the data to the data fork when the inode is in local format, it 
never let the
local format inode to go down the writeback path.

The xfs_writepage_map will continue as if nothing happens when 
xfs_inode_extents_to_local
returns 0.

Thanks
Shan Hai
> --D
>
>> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp);
>> +	if (error)
>> +		return error;
>> +
>> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +	xfs_trans_ijoin(tp, ip, 0);
>> +
>> +	xfs_defer_init(&dfops, &first_block);
>> +
>> +	logflags = 0;
>> +	error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
>> +			XFS_DATA_FORK, page);
>> +	if (error)
>> +		goto trans_cancel;
>> +
>> +	xfs_trans_log_inode(tp, ip, logflags);
>> +	error = xfs_defer_finish(&tp, &dfops);
>> +	if (error)
>> +		goto trans_cancel;
>> +	error = xfs_trans_commit(tp);
>> +	if (error)
>> +		goto error0;
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +
>> +	return 0;
>> +
>> +trans_cancel:
>> +	xfs_defer_cancel(&dfops);
>> +	xfs_trans_cancel(tp);
>> +error0:
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +	return error;
>> +}
>>   /*
>>    * We implement an immediate ioend submission policy here to avoid needing to
>>    * chain multiple ioends and hence nest mempool allocations which can violate
>> @@ -920,6 +970,8 @@ xfs_writepage_map(
>>   	int			count = 0;
>>   	int			uptodate = 1;
>>   	unsigned int		new_type;
>> +	xfs_inode_t		*ip = XFS_I(inode);
>> +	struct xfs_mount	*mp = ip->i_mount;
>>   
>>   	bh = head = page_buffers(page);
>>   	offset = page_offset(page);
>> @@ -1044,6 +1096,14 @@ xfs_writepage_map(
>>   		end_page_writeback(page);
>>   	}
>>   
>> +	if (!error) {
>> +		if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +				i_size_read(inode) <= XFS_IFORK_DSIZE(ip)) {
>> +			error = xfs_inode_extents_to_local(ip, XFS_DATA_FORK,
>> +								page);
>> +		}
>> +	}
>> +
>>   	mapping_set_error(page->mapping, error);
>>   	return error;
>>   }
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 4/8] xfs: implement inline data read write code

[Shan Hai]

Implement read/write functions for inline data access and use them
for buffered/DIO/DAX operations.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/xfs_file.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 242 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a3e7767a5715..55047928b720 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -27,6 +27,7 @@
 #include "xfs_pnfs.h"
 #include "xfs_iomap.h"
 #include "xfs_reflink.h"
+#include "xfs_defer.h"
 
 #include <linux/dcache.h>
 #include <linux/falloc.h>
@@ -175,6 +176,185 @@ xfs_file_fsync(
 	return error;
 }
 
+/*
+ * Read the inline data of a local format file.
+ */
+STATIC ssize_t
+xfs_file_inline_data_read(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	struct kiocb		*iocb,
+	struct iov_iter		*to)
+{
+	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct inode		*inode = VFS_I(ip);
+	loff_t			isize = i_size_read(inode);
+	loff_t			*ppos = &iocb->ki_pos;
+	size_t			count = iov_iter_count(to);
+	ssize_t			ret = 0;
+
+	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
+	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
+
+	if (!count)
+		return 0;
+
+	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
+		return 0;
+
+	if (*ppos > isize)
+		return 0;
+
+	iov_iter_truncate(to, inode->i_sb->s_maxbytes);
+	count = (*ppos + count > isize) ? isize - *ppos : count;
+	ret = copy_to_iter(ifp->if_u1.if_data + *ppos, count, to);
+	*ppos += ret;
+
+	return ret;
+}
+
+/*
+ * Try to inline the data into the xfs inode data fork when it has enough
+ * space to hold it. The data inlining happens at below points:
+ *
+ * - writeback: for buffered writes
+ * - write_iter: for dax/dio writes
+ *
+ * The extents to local format conversion is done here for DAX/DIO write,
+ * while the same conversion is done in the writeback path for buffered write.
+ * This function also converts inode from local to extents when the data
+ * fork could not hold the data inline anymore.
+ */
+STATIC ssize_t
+xfs_file_inline_data_write(
+	struct xfs_inode	*ip,
+	int			whichfork,
+	struct kiocb		*iocb,
+	struct iov_iter		*from)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
+	struct inode		*inode = VFS_I(ip);
+	struct address_space	*mapping = inode->i_mapping;
+	loff_t			pos = iocb->ki_pos;
+	size_t			count = iov_iter_count(from);
+	struct xfs_trans	*tp;
+	struct xfs_defer_ops	dfops;
+	xfs_fsblock_t		first_block;
+	int			ret;
+	int			error = 0;
+	int			logflags = 0;
+	loff_t			written = 0;
+	xfs_fileoff_t		bno = 0;
+	struct xfs_bmbt_irec	map;
+	int			nmap;
+	struct page		*page;
+	char			*kaddr;
+
+	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
+	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 1, 0, 0, &tp);
+	if (error) {
+		ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
+		return error;
+	}
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, 0);
+	xfs_defer_init(&dfops, &first_block);
+
+	if (pos + count > XFS_IFORK_DSIZE(ip)) {
+		if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS) {
+			error = 0;
+			goto trans_cancel;
+		}
+
+		page = find_or_create_page(mapping, pos >> PAGE_SHIFT, GFP_NOFS);
+		if (!page) {
+			error = -ENOMEM;
+			goto trans_cancel;
+		}
+		kaddr = kmap_atomic(page);
+		memcpy(kaddr, ifp->if_u1.if_data, ifp->if_bytes);
+		kunmap_atomic(kaddr);
+		SetPageUptodate(page);
+		unlock_page(page);
+		put_page(page);
+
+		xfs_bmap_forkoff_reset(ip, whichfork);
+		ifp->if_flags &= ~XFS_IFINLINE;
+		ifp->if_flags |= XFS_IFEXTENTS;
+		ifp->if_u1.if_root = NULL;
+		ifp->if_height = 0;
+		ifp->if_bytes = 0;
+		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
+
+		written = ifp->if_bytes;
+		error = xfs_bmap_first_unused(tp, ip, 1, &bno, whichfork);
+		if (error)
+			goto trans_cancel;
+		nmap = 1;
+		error = xfs_bmapi_write(tp, ip, bno, 1, XFS_BMAPI_ENTIRE,
+				&first_block, 1, &map, &nmap, &dfops);
+		if (error)
+			goto trans_cancel;
+
+		goto out_local_to_extents;
+	}
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
+		xfs_idata_realloc(ip, (pos + count) - ifp->if_bytes, whichfork);
+		ret = copy_from_iter(ifp->if_u1.if_data + pos, count, from);
+		written = ret;
+		goto out;
+	}
+
+	if (IS_DAX(inode) || iocb->ki_flags & IOCB_DIRECT) {
+		struct page	*page = alloc_page(GFP_KERNEL | GFP_NOFS);
+		if (!page) {
+			error = -ENOMEM;
+			goto trans_cancel;
+		}
+
+		kaddr = kmap_atomic(page);
+		ret = copy_from_iter(kaddr + pos, count, from);
+		kunmap_atomic(kaddr);
+		written = ret;
+		error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
+							XFS_DATA_FORK, page);
+		if (error) {
+			__free_page(page);
+			goto trans_cancel;
+		}
+
+		__free_page(page);
+		goto out;
+	}
+trans_cancel:
+	xfs_defer_cancel(&dfops);
+	xfs_trans_cancel(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+out:
+	if (pos + written > i_size_read(inode))
+		i_size_write(inode, pos + written);
+	ip->i_d.di_size = pos + written;
+
+out_local_to_extents:
+	logflags |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
+	xfs_trans_log_inode(tp, ip, logflags);
+
+	error = xfs_defer_finish(&tp, &dfops);
+	if (error)
+		goto trans_cancel;
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
+	return written > 0 ? written : error;
+}
+
+
 STATIC ssize_t
 xfs_file_dio_aio_read(
 	struct kiocb		*iocb,
@@ -192,7 +372,12 @@ xfs_file_dio_aio_read(
 	file_accessed(iocb->ki_filp);
 
 	xfs_ilock(ip, XFS_IOLOCK_SHARED);
-	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
+	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
+	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
+		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
+	} else {
+		ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
+	}
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
 	return ret;
@@ -219,6 +404,13 @@ xfs_file_dax_read(
 		xfs_ilock(ip, XFS_IOLOCK_SHARED);
 	}
 
+	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
+	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
+		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
+	} else {
+		ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
+	}
+
 	ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
@@ -242,7 +434,12 @@ xfs_file_buffered_aio_read(
 	} else {
 		xfs_ilock(ip, XFS_IOLOCK_SHARED);
 	}
-	ret = generic_file_read_iter(iocb, to);
+
+	if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
+		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
+	} else {
+		ret = generic_file_read_iter(iocb, to);
+	}
 	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
 
 	return ret;
@@ -487,6 +684,7 @@ xfs_file_dio_aio_write(
 	size_t			count = iov_iter_count(from);
 	struct xfs_buftarg      *target = XFS_IS_REALTIME_INODE(ip) ?
 					mp->m_rtdev_targp : mp->m_ddev_targp;
+	int			error;
 
 	/* DIO must be aligned to device logical sector size */
 	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
@@ -547,6 +745,22 @@ xfs_file_dio_aio_write(
 	}
 
 	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
+
+	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&
+		S_ISREG(inode->i_mode)) {
+		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb,
+							from);
+		if (error) {
+			ret = error;
+			goto out;
+		}
+		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
+			ret = count;
+			goto out;
+		}
+	}
+
 	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
 out:
 	xfs_iunlock(ip, iolock);
@@ -586,6 +800,20 @@ xfs_file_dax_write(
 	count = iov_iter_count(from);
 
 	trace_xfs_file_dax_write(ip, count, pos);
+
+	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
+		ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
+		S_ISREG(inode->i_mode)) {
+		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK,
+							iocb, from);
+		if (error)
+			goto out;
+		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
+			ret = count;
+			goto out;
+		}
+	}
+
 	ret = dax_iomap_rw(iocb, from, &xfs_iomap_ops);
 	if (ret > 0 && iocb->ki_pos > i_size_read(inode)) {
 		i_size_write(inode, iocb->ki_pos);
@@ -614,6 +842,7 @@ xfs_file_buffered_aio_write(
 	struct address_space	*mapping = file->f_mapping;
 	struct inode		*inode = mapping->host;
 	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
 	ssize_t			ret;
 	int			enospc = 0;
 	int			iolock;
@@ -629,6 +858,17 @@ xfs_file_buffered_aio_write(
 	if (ret)
 		goto out;
 
+	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE) {
+		ret = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb, from);
+		if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) ==
+							XFS_DINODE_FMT_LOCAL){
+			if (likely(ret >= 0))
+				iocb->ki_pos += ret;
+			goto out;
+		}
+	}
+
 	/* We can write back this queue in page reclaim */
 	current->backing_dev_info = inode_to_bdi(inode);
 
-- 
2.11.0

Re: [PATCH RFC 4/8] xfs: implement inline data read write code

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:25AM +0800, Shan Hai wrote:
> Implement read/write functions for inline data access and use them
> for buffered/DIO/DAX operations.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_file.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 242 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index a3e7767a5715..55047928b720 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -27,6 +27,7 @@
>  #include "xfs_pnfs.h"
>  #include "xfs_iomap.h"
>  #include "xfs_reflink.h"
> +#include "xfs_defer.h"
>  
>  #include <linux/dcache.h>
>  #include <linux/falloc.h>
> @@ -175,6 +176,185 @@ xfs_file_fsync(
>  	return error;
>  }
>  
> +/*
> + * Read the inline data of a local format file.
> + */
> +STATIC ssize_t
> +xfs_file_inline_data_read(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct kiocb		*iocb,
> +	struct iov_iter		*to)
> +{
> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct inode		*inode = VFS_I(ip);
> +	loff_t			isize = i_size_read(inode);
> +	loff_t			*ppos = &iocb->ki_pos;
> +	size_t			count = iov_iter_count(to);
> +	ssize_t			ret = 0;
> +
> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
> +
> +	if (!count)
> +		return 0;
> +
> +	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
> +		return 0;
> +
> +	if (*ppos > isize)
> +		return 0;
> +
> +	iov_iter_truncate(to, inode->i_sb->s_maxbytes);
> +	count = (*ppos + count > isize) ? isize - *ppos : count;
> +	ret = copy_to_iter(ifp->if_u1.if_data + *ppos, count, to);
> +	*ppos += ret;
> +
> +	return ret;
> +}
> +
> +/*
> + * Try to inline the data into the xfs inode data fork when it has enough
> + * space to hold it. The data inlining happens at below points:
> + *
> + * - writeback: for buffered writes
> + * - write_iter: for dax/dio writes
> + *
> + * The extents to local format conversion is done here for DAX/DIO write,
> + * while the same conversion is done in the writeback path for buffered write.
> + * This function also converts inode from local to extents when the data
> + * fork could not hold the data inline anymore.
> + */
> +STATIC ssize_t
> +xfs_file_inline_data_write(
> +	struct xfs_inode	*ip,
> +	int			whichfork,
> +	struct kiocb		*iocb,
> +	struct iov_iter		*from)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
> +	struct inode		*inode = VFS_I(ip);
> +	struct address_space	*mapping = inode->i_mapping;
> +	loff_t			pos = iocb->ki_pos;
> +	size_t			count = iov_iter_count(from);
> +	struct xfs_trans	*tp;
> +	struct xfs_defer_ops	dfops;
> +	xfs_fsblock_t		first_block;
> +	int			ret;
> +	int			error = 0;
> +	int			logflags = 0;
> +	loff_t			written = 0;
> +	xfs_fileoff_t		bno = 0;
> +	struct xfs_bmbt_irec	map;
> +	int			nmap;
> +	struct page		*page;
> +	char			*kaddr;
> +
> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
> +
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 1, 0, 0, &tp);
> +	if (error) {
> +		ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +	xfs_defer_init(&dfops, &first_block);
> +
> +	if (pos + count > XFS_IFORK_DSIZE(ip)) {
> +		if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS) {
> +			error = 0;
> +			goto trans_cancel;
> +		}
> +
> +		page = find_or_create_page(mapping, pos >> PAGE_SHIFT, GFP_NOFS);
> +		if (!page) {
> +			error = -ENOMEM;
> +			goto trans_cancel;
> +		}
> +		kaddr = kmap_atomic(page);
> +		memcpy(kaddr, ifp->if_u1.if_data, ifp->if_bytes);
> +		kunmap_atomic(kaddr);
> +		SetPageUptodate(page);
> +		unlock_page(page);
> +		put_page(page);
> +
> +		xfs_bmap_forkoff_reset(ip, whichfork);
> +		ifp->if_flags &= ~XFS_IFINLINE;
> +		ifp->if_flags |= XFS_IFEXTENTS;
> +		ifp->if_u1.if_root = NULL;
> +		ifp->if_height = 0;
> +		ifp->if_bytes = 0;
> +		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> +
> +		written = ifp->if_bytes;
> +		error = xfs_bmap_first_unused(tp, ip, 1, &bno, whichfork);
> +		if (error)
> +			goto trans_cancel;
> +		nmap = 1;
> +		error = xfs_bmapi_write(tp, ip, bno, 1, XFS_BMAPI_ENTIRE,
> +				&first_block, 1, &map, &nmap, &dfops);
> +		if (error)
> +			goto trans_cancel;
> +
> +		goto out_local_to_extents;
> +	}
> +
> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
> +		xfs_idata_realloc(ip, (pos + count) - ifp->if_bytes, whichfork);
> +		ret = copy_from_iter(ifp->if_u1.if_data + pos, count, from);
> +		written = ret;
> +		goto out;
> +	}
> +
> +	if (IS_DAX(inode) || iocb->ki_flags & IOCB_DIRECT) {
> +		struct page	*page = alloc_page(GFP_KERNEL | GFP_NOFS);
> +		if (!page) {
> +			error = -ENOMEM;
> +			goto trans_cancel;
> +		}
> +
> +		kaddr = kmap_atomic(page);
> +		ret = copy_from_iter(kaddr + pos, count, from);
> +		kunmap_atomic(kaddr);
> +		written = ret;
> +		error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
> +							XFS_DATA_FORK, page);
> +		if (error) {
> +			__free_page(page);
> +			goto trans_cancel;
> +		}
> +
> +		__free_page(page);
> +		goto out;
> +	}
> +trans_cancel:
> +	xfs_defer_cancel(&dfops);
> +	xfs_trans_cancel(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return error;
> +out:
> +	if (pos + written > i_size_read(inode))
> +		i_size_write(inode, pos + written);
> +	ip->i_d.di_size = pos + written;
> +
> +out_local_to_extents:
> +	logflags |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
> +	xfs_trans_log_inode(tp, ip, logflags);
> +
> +	error = xfs_defer_finish(&tp, &dfops);
> +	if (error)
> +		goto trans_cancel;
> +	error = xfs_trans_commit(tp);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +
> +	return written > 0 ? written : error;
> +}
> +
> +
>  STATIC ssize_t
>  xfs_file_dio_aio_read(
>  	struct kiocb		*iocb,
> @@ -192,7 +372,12 @@ xfs_file_dio_aio_read(
>  	file_accessed(iocb->ki_filp);
>  
>  	xfs_ilock(ip, XFS_IOLOCK_SHARED);
> -	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);

I thought iomap was growing the ability to handle inline data?  At least
for gfs2?

Hmm, you might ask Christoph about how to support that... also these
read/write paths are about to get a lot of rip/smash this cycle.

> +	} else {
> +		ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
> +	}
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
>  	return ret;
> @@ -219,6 +404,13 @@ xfs_file_dax_read(
>  		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>  	}
>  
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
> +	} else {
> +		ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
> +	}
> +
>  	ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
> @@ -242,7 +434,12 @@ xfs_file_buffered_aio_read(
>  	} else {
>  		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>  	}
> -	ret = generic_file_read_iter(iocb, to);
> +
> +	if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
> +	} else {
> +		ret = generic_file_read_iter(iocb, to);
> +	}
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>  
>  	return ret;
> @@ -487,6 +684,7 @@ xfs_file_dio_aio_write(
>  	size_t			count = iov_iter_count(from);
>  	struct xfs_buftarg      *target = XFS_IS_REALTIME_INODE(ip) ?
>  					mp->m_rtdev_targp : mp->m_ddev_targp;
> +	int			error;
>  
>  	/* DIO must be aligned to device logical sector size */
>  	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
> @@ -547,6 +745,22 @@ xfs_file_dio_aio_write(
>  	}
>  
>  	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
> +
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&

!= BTREE? Why not == INLINE?

> +		S_ISREG(inode->i_mode)) {
> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb,
> +							from);

int != ssize_t... also, if we decided we needed extents format but the
file turned out to be extents format already then did we lose the write
here?

> +		if (error) {
> +			ret = error;
> +			goto out;
> +		}
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +			ret = count;
> +			goto out;
> +		}
> +	}
> +
>  	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
>  out:
>  	xfs_iunlock(ip, iolock);
> @@ -586,6 +800,20 @@ xfs_file_dax_write(
>  	count = iov_iter_count(from);
>  
>  	trace_xfs_file_dax_write(ip, count, pos);
> +
> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
> +		ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
> +		S_ISREG(inode->i_mode)) {
> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK,
> +							iocb, from);
> +		if (error)
> +			goto out;
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +			ret = count;
> +			goto out;
> +		}
> +	}
> +
>  	ret = dax_iomap_rw(iocb, from, &xfs_iomap_ops);
>  	if (ret > 0 && iocb->ki_pos > i_size_read(inode)) {
>  		i_size_write(inode, iocb->ki_pos);
> @@ -614,6 +842,7 @@ xfs_file_buffered_aio_write(
>  	struct address_space	*mapping = file->f_mapping;
>  	struct inode		*inode = mapping->host;
>  	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
>  	ssize_t			ret;
>  	int			enospc = 0;
>  	int			iolock;
> @@ -629,6 +858,17 @@ xfs_file_buffered_aio_write(
>  	if (ret)
>  		goto out;
>  
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE) {
> +		ret = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb, from);
> +		if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) ==
> +							XFS_DINODE_FMT_LOCAL){

What's going on with the indenting here?

--D

> +			if (likely(ret >= 0))
> +				iocb->ki_pos += ret;
> +			goto out;
> +		}
> +	}
> +
>  	/* We can write back this queue in page reclaim */
>  	current->backing_dev_info = inode_to_bdi(inode);
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 4/8] xfs: implement inline data read write code

[Shan Hai]

On 2018年07月06日 11:33, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:25AM +0800, Shan Hai wrote:
>> Implement read/write functions for inline data access and use them
>> for buffered/DIO/DAX operations.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_file.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 242 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> index a3e7767a5715..55047928b720 100644
>> --- a/fs/xfs/xfs_file.c
>> +++ b/fs/xfs/xfs_file.c
>> @@ -27,6 +27,7 @@
>>   #include "xfs_pnfs.h"
>>   #include "xfs_iomap.h"
>>   #include "xfs_reflink.h"
>> +#include "xfs_defer.h"
>>   
>>   #include <linux/dcache.h>
>>   #include <linux/falloc.h>
>> @@ -175,6 +176,185 @@ xfs_file_fsync(
>>   	return error;
>>   }
>>   
>> +/*
>> + * Read the inline data of a local format file.
>> + */
>> +STATIC ssize_t
>> +xfs_file_inline_data_read(
>> +	struct xfs_inode	*ip,
>> +	int			whichfork,
>> +	struct kiocb		*iocb,
>> +	struct iov_iter		*to)
>> +{
>> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
>> +	struct inode		*inode = VFS_I(ip);
>> +	loff_t			isize = i_size_read(inode);
>> +	loff_t			*ppos = &iocb->ki_pos;
>> +	size_t			count = iov_iter_count(to);
>> +	ssize_t			ret = 0;
>> +
>> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
>> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
>> +
>> +	if (!count)
>> +		return 0;
>> +
>> +	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
>> +		return 0;
>> +
>> +	if (*ppos > isize)
>> +		return 0;
>> +
>> +	iov_iter_truncate(to, inode->i_sb->s_maxbytes);
>> +	count = (*ppos + count > isize) ? isize - *ppos : count;
>> +	ret = copy_to_iter(ifp->if_u1.if_data + *ppos, count, to);
>> +	*ppos += ret;
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Try to inline the data into the xfs inode data fork when it has enough
>> + * space to hold it. The data inlining happens at below points:
>> + *
>> + * - writeback: for buffered writes
>> + * - write_iter: for dax/dio writes
>> + *
>> + * The extents to local format conversion is done here for DAX/DIO write,
>> + * while the same conversion is done in the writeback path for buffered write.
>> + * This function also converts inode from local to extents when the data
>> + * fork could not hold the data inline anymore.
>> + */
>> +STATIC ssize_t
>> +xfs_file_inline_data_write(
>> +	struct xfs_inode	*ip,
>> +	int			whichfork,
>> +	struct kiocb		*iocb,
>> +	struct iov_iter		*from)
>> +{
>> +	struct xfs_mount	*mp = ip->i_mount;
>> +	xfs_ifork_t		*ifp = XFS_IFORK_PTR(ip, whichfork);
>> +	struct inode		*inode = VFS_I(ip);
>> +	struct address_space	*mapping = inode->i_mapping;
>> +	loff_t			pos = iocb->ki_pos;
>> +	size_t			count = iov_iter_count(from);
>> +	struct xfs_trans	*tp;
>> +	struct xfs_defer_ops	dfops;
>> +	xfs_fsblock_t		first_block;
>> +	int			ret;
>> +	int			error = 0;
>> +	int			logflags = 0;
>> +	loff_t			written = 0;
>> +	xfs_fileoff_t		bno = 0;
>> +	struct xfs_bmbt_irec	map;
>> +	int			nmap;
>> +	struct page		*page;
>> +	char			*kaddr;
>> +
>> +	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS ||
>> +	       XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
>> +
>> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 1, 0, 0, &tp);
>> +	if (error) {
>> +		ASSERT(error == -ENOSPC || XFS_FORCED_SHUTDOWN(mp));
>> +		return error;
>> +	}
>> +
>> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +	xfs_trans_ijoin(tp, ip, 0);
>> +	xfs_defer_init(&dfops, &first_block);
>> +
>> +	if (pos + count > XFS_IFORK_DSIZE(ip)) {
>> +		if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS) {
>> +			error = 0;
>> +			goto trans_cancel;
>> +		}
>> +
>> +		page = find_or_create_page(mapping, pos >> PAGE_SHIFT, GFP_NOFS);
>> +		if (!page) {
>> +			error = -ENOMEM;
>> +			goto trans_cancel;
>> +		}
>> +		kaddr = kmap_atomic(page);
>> +		memcpy(kaddr, ifp->if_u1.if_data, ifp->if_bytes);
>> +		kunmap_atomic(kaddr);
>> +		SetPageUptodate(page);
>> +		unlock_page(page);
>> +		put_page(page);
>> +
>> +		xfs_bmap_forkoff_reset(ip, whichfork);
>> +		ifp->if_flags &= ~XFS_IFINLINE;
>> +		ifp->if_flags |= XFS_IFEXTENTS;
>> +		ifp->if_u1.if_root = NULL;
>> +		ifp->if_height = 0;
>> +		ifp->if_bytes = 0;
>> +		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
>> +
>> +		written = ifp->if_bytes;
>> +		error = xfs_bmap_first_unused(tp, ip, 1, &bno, whichfork);
>> +		if (error)
>> +			goto trans_cancel;
>> +		nmap = 1;
>> +		error = xfs_bmapi_write(tp, ip, bno, 1, XFS_BMAPI_ENTIRE,
>> +				&first_block, 1, &map, &nmap, &dfops);
>> +		if (error)
>> +			goto trans_cancel;
>> +
>> +		goto out_local_to_extents;
>> +	}
>> +
>> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
>> +		xfs_idata_realloc(ip, (pos + count) - ifp->if_bytes, whichfork);
>> +		ret = copy_from_iter(ifp->if_u1.if_data + pos, count, from);
>> +		written = ret;
>> +		goto out;
>> +	}
>> +
>> +	if (IS_DAX(inode) || iocb->ki_flags & IOCB_DIRECT) {
>> +		struct page	*page = alloc_page(GFP_KERNEL | GFP_NOFS);
>> +		if (!page) {
>> +			error = -ENOMEM;
>> +			goto trans_cancel;
>> +		}
>> +
>> +		kaddr = kmap_atomic(page);
>> +		ret = copy_from_iter(kaddr + pos, count, from);
>> +		kunmap_atomic(kaddr);
>> +		written = ret;
>> +		error = xfs_bmap_extents_to_local(tp, ip, &dfops, &logflags,
>> +							XFS_DATA_FORK, page);
>> +		if (error) {
>> +			__free_page(page);
>> +			goto trans_cancel;
>> +		}
>> +
>> +		__free_page(page);
>> +		goto out;
>> +	}
>> +trans_cancel:
>> +	xfs_defer_cancel(&dfops);
>> +	xfs_trans_cancel(tp);
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +	return error;
>> +out:
>> +	if (pos + written > i_size_read(inode))
>> +		i_size_write(inode, pos + written);
>> +	ip->i_d.di_size = pos + written;
>> +
>> +out_local_to_extents:
>> +	logflags |= (XFS_ILOG_DDATA | XFS_ILOG_CORE);
>> +	xfs_trans_log_inode(tp, ip, logflags);
>> +
>> +	error = xfs_defer_finish(&tp, &dfops);
>> +	if (error)
>> +		goto trans_cancel;
>> +	error = xfs_trans_commit(tp);
>> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> +
>> +	return written > 0 ? written : error;
>> +}
>> +
>> +
>>   STATIC ssize_t
>>   xfs_file_dio_aio_read(
>>   	struct kiocb		*iocb,
>> @@ -192,7 +372,12 @@ xfs_file_dio_aio_read(
>>   	file_accessed(iocb->ki_filp);
>>   
>>   	xfs_ilock(ip, XFS_IOLOCK_SHARED);
>> -	ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
>> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
>> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
> I thought iomap was growing the ability to handle inline data?  At least
> for gfs2?
>
> Hmm, you might ask Christoph about how to support that... also these
> read/write paths are about to get a lot of rip/smash this cycle.
>
>> +	} else {
>> +		ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL);
>> +	}
>>   	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>>   
>>   	return ret;
>> @@ -219,6 +404,13 @@ xfs_file_dax_read(
>>   		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>>   	}
>>   
>> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
>> +	    XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
>> +	} else {
>> +		ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
>> +	}
>> +
>>   	ret = dax_iomap_rw(iocb, to, &xfs_iomap_ops);
>>   	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>>   
>> @@ -242,7 +434,12 @@ xfs_file_buffered_aio_read(
>>   	} else {
>>   		xfs_ilock(ip, XFS_IOLOCK_SHARED);
>>   	}
>> -	ret = generic_file_read_iter(iocb, to);
>> +
>> +	if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>> +		ret = xfs_file_inline_data_read(ip, XFS_DATA_FORK, iocb, to);
>> +	} else {
>> +		ret = generic_file_read_iter(iocb, to);
>> +	}
>>   	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
>>   
>>   	return ret;
>> @@ -487,6 +684,7 @@ xfs_file_dio_aio_write(
>>   	size_t			count = iov_iter_count(from);
>>   	struct xfs_buftarg      *target = XFS_IS_REALTIME_INODE(ip) ?
>>   					mp->m_rtdev_targp : mp->m_ddev_targp;
>> +	int			error;
>>   
>>   	/* DIO must be aligned to device logical sector size */
>>   	if ((iocb->ki_pos | count) & target->bt_logical_sectormask)
>> @@ -547,6 +745,22 @@ xfs_file_dio_aio_write(
>>   	}
>>   
>>   	trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
>> +
>> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&
> != BTREE? Why not == INLINE?

The xfs inode is born with EXTENTS format and the very first write to a 
file will
encounter the EXTENTS since it's not converted to INLINE yet, the format
conversion happens in the following xfs_file_inline_data_write.

>> +		S_ISREG(inode->i_mode)) {
>> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb,
>> +							from);
> int != ssize_t... also, if we decided we needed extents format but the
> file turned out to be extents format already then did we lose the write
> here?

If the file is already in the extents format then the above 
xfs_file_inline_data_write
just returns without format conversion and the control flow falls 
through to the normal
extents handling because of the following if statement found that the 
format is not LOCAL.

>> +		if (error) {
>> +			ret = error;
>> +			goto out;
>> +		}
>> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
>> +			ret = count;
>> +			goto out;
>> +		}
>> +	}
>> +
>>   	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
>>   out:
>>   	xfs_iunlock(ip, iolock);
>> @@ -586,6 +800,20 @@ xfs_file_dax_write(
>>   	count = iov_iter_count(from);
>>   
>>   	trace_xfs_file_dax_write(ip, count, pos);
>> +
>> +	if (xfs_sb_version_hasinlinedata(&ip->i_mount->m_sb) &&
>> +		ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
>> +		S_ISREG(inode->i_mode)) {
>> +		error = xfs_file_inline_data_write(ip, XFS_DATA_FORK,
>> +							iocb, from);
>> +		if (error)
>> +			goto out;
>> +		if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
>> +			ret = count;
>> +			goto out;
>> +		}
>> +	}
>> +
>>   	ret = dax_iomap_rw(iocb, from, &xfs_iomap_ops);
>>   	if (ret > 0 && iocb->ki_pos > i_size_read(inode)) {
>>   		i_size_write(inode, iocb->ki_pos);
>> @@ -614,6 +842,7 @@ xfs_file_buffered_aio_write(
>>   	struct address_space	*mapping = file->f_mapping;
>>   	struct inode		*inode = mapping->host;
>>   	struct xfs_inode	*ip = XFS_I(inode);
>> +	struct xfs_mount	*mp = ip->i_mount;
>>   	ssize_t			ret;
>>   	int			enospc = 0;
>>   	int			iolock;
>> @@ -629,6 +858,17 @@ xfs_file_buffered_aio_write(
>>   	if (ret)
>>   		goto out;
>>   
>> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE) {
>> +		ret = xfs_file_inline_data_write(ip, XFS_DATA_FORK, iocb, from);
>> +		if (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) ==
>> +							XFS_DINODE_FMT_LOCAL){
> What's going on with the indenting here?

Oops, sorry, I will fix it.

Thanks
Shan Hai
>
> --D
>
>> +			if (likely(ret >= 0))
>> +				iocb->ki_pos += ret;
>> +			goto out;
>> +		}
>> +	}
>> +
>>   	/* We can write back this queue in page reclaim */
>>   	current->backing_dev_info = inode_to_bdi(inode);
>>   
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 4/8] xfs: implement inline data read write code

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 11:12:25AM +0800, Shan Hai wrote:
> Implement read/write functions for inline data access and use them
> for buffered/DIO/DAX operations.

Please use the iomap inline handling in this branch instead of
implementing it in XFS:

https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/log/?h=iomap-4.19-merge

It seems like your code doesn't use the page cache for inline data
at the moment, which means you'd want to always use the direct I/O
version of the code as-is.

Althought that makes me wonder how you handle memory mapped I/O, but
I guess I'll find out later.

Re: [PATCH RFC 4/8] xfs: implement inline data read write code

[Shan Hai]

On 2018年07月08日 23:45, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 11:12:25AM +0800, Shan Hai wrote:
>> Implement read/write functions for inline data access and use them
>> for buffered/DIO/DAX operations.
> Please use the iomap inline handling in this branch instead of
> implementing it in XFS:
>
> https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/log/?h=iomap-4.19-merge

OK, I will base the patches on the iomap inline handling code.

> It seems like your code doesn't use the page cache for inline data
> at the moment, which means you'd want to always use the direct I/O
> version of the code as-is.

Yes, the read path uses direct I/O version while the write is not.

> Althought that makes me wonder how you handle memory mapped I/O, but
> I guess I'll find out later.

I will use page cache to handle the mmap case, thanks for the review.

Thanks
Shan Hai

[PATCH RFC 5/8] xfs: consider the local format inode in misc operations

[Shan Hai]

The local format inode is a legal citizen from now on and consider
it in misc operations.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c      | 18 ++++++++++--------
 fs/xfs/libxfs/xfs_inode_buf.c |  6 ------
 fs/xfs/scrub/inode.c          |  2 +-
 fs/xfs/xfs_bmap_util.c        | 11 +++++++++--
 fs/xfs/xfs_inode.c            | 20 ++++++++++++++------
 fs/xfs/xfs_inode_item.c       |  5 ++++-
 fs/xfs/xfs_iomap.c            |  5 +++--
 fs/xfs/xfs_log_recover.c      |  3 ++-
 8 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index bea6dc254a7d..6b151bd15da7 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -830,11 +830,6 @@ xfs_bmap_local_to_extents(
 	struct xfs_bmbt_irec rec;
 	struct xfs_iext_cursor icur;
 
-	/*
-	 * We don't want to deal with the case of keeping inode data inline yet.
-	 * So sending the data fork of a regular inode is invalid.
-	 */
-	ASSERT(!(S_ISREG(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK));
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
 
@@ -3840,7 +3835,8 @@ xfs_bmapi_read(
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
-	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
+	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
+	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),
 	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
 		XFS_ERROR_REPORT("xfs_bmapi_read", XFS_ERRLEVEL_LOW, mp);
 		return -EFSCORRUPTED;
@@ -3863,7 +3859,7 @@ xfs_bmapi_read(
 		return 0;
 	}
 
-	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+	if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
 		error = xfs_iread_extents(NULL, ip, whichfork);
 		if (error)
 			return error;
@@ -4285,7 +4281,6 @@ xfs_bmapi_write(
 	       (flags & (XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK)) ==
 			(XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK));
 	ASSERT(len > 0);
-	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!(flags & XFS_BMAPI_REMAP));
 
@@ -5244,11 +5239,18 @@ __xfs_bunmapi(
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 	if (unlikely(
 	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
+	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
 	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
 		XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
 				 ip->i_mount);
 		return -EFSCORRUPTED;
 	}
+
+	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
+		*rlen = 0;
+		return 0;
+	}
+
 	mp = ip->i_mount;
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 33dc34655ac3..cb3c4b308137 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -157,7 +157,6 @@ const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
 	.verify_write = xfs_inode_buf_write_verify,
 };
 
-
 /*
  * This routine is called to map an inode to the buffer containing the on-disk
  * version of the inode.  It returns a pointer to the buffer containing the
@@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
 
 	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
 	case XFS_DINODE_FMT_LOCAL:
-		/*
-		 * no local regular files yet
-		 */
 		if (whichfork == XFS_DATA_FORK) {
-			if (S_ISREG(be16_to_cpu(dip->di_mode)))
-				return __this_address;
 			if (be64_to_cpu(dip->di_size) >
 					XFS_DFORK_SIZE(dip, mp, whichfork))
 				return __this_address;
diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index 7a6208505980..1501983dd7aa 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -283,7 +283,7 @@ xfs_scrub_dinode(
 			xfs_scrub_ino_set_corrupt(sc, ino);
 		break;
 	case XFS_DINODE_FMT_LOCAL:
-		if (!S_ISDIR(mode) && !S_ISLNK(mode))
+		if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
 			xfs_scrub_ino_set_corrupt(sc, ino);
 		break;
 	case XFS_DINODE_FMT_EXTENTS:
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 83b1e8c6c18f..46f718177fd7 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -760,7 +760,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
 		return false;
 
 	/* If we haven't read in the extent list, then don't do it now. */
-	if (!(ip->i_df.if_flags & XFS_IFEXTENTS))
+	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL &&
+		!(ip->i_df.if_flags & XFS_IFEXTENTS))
 		return false;
 
 	/*
@@ -768,7 +769,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
 	 * has delalloc blocks and we are forced to remove them.
 	 */
 	if (ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND))
-		if (!force || ip->i_delayed_blks == 0)
+		if ((ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) &&
+			(!force || ip->i_delayed_blks == 0))
 			return false;
 
 	return true;
@@ -792,6 +794,11 @@ xfs_free_eofblocks(
 	struct xfs_bmbt_irec	imap;
 	struct xfs_mount	*mp = ip->i_mount;
 
+	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
+		ip->i_delayed_blks = 0;
+		return 0;
+	}
 	/*
 	 * Figure out if there are any blocks beyond the end
 	 * of the file.  If not, then there is nothing to do.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5df4de666cc1..78b9790a7cd4 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1563,7 +1563,6 @@ xfs_itruncate_extents_flags(
 	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
 
 	trace_xfs_itruncate_extents_start(ip, new_size);
-
 	flags |= xfs_bmapi_aflag(whichfork);
 
 	/*
@@ -1745,9 +1744,15 @@ xfs_inactive_truncate(
 	ip->i_d.di_size = 0;
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-	error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
-	if (error)
-		goto error_trans_cancel;
+	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
+		ASSERT(ip->i_d.di_nextents == 0);
+		ip->i_delayed_blks = 0;
+	} else {
+		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
+		if (error)
+			goto error_trans_cancel;
+	}
 
 	ASSERT(ip->i_d.di_nextents == 0);
 
@@ -1879,6 +1884,7 @@ xfs_inactive(
 	if (VFS_I(ip)->i_mode == 0) {
 		ASSERT(ip->i_df.if_real_bytes == 0);
 		ASSERT(ip->i_df.if_broot_bytes == 0);
+		ASSERT(ip->i_delayed_blks == 0);
 		return;
 	}
 
@@ -1911,8 +1917,9 @@ xfs_inactive(
 
 	if (S_ISREG(VFS_I(ip)->i_mode) &&
 	    (ip->i_d.di_size != 0 || XFS_ISIZE(ip) != 0 ||
-	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
+	     ip->i_d.di_nextents >= 0 || ip->i_delayed_blks >= 0)) {
 		truncate = 1;
+	}
 
 	error = xfs_qm_dqattach(ip);
 	if (error)
@@ -3554,7 +3561,8 @@ xfs_iflush_int(
 	if (S_ISREG(VFS_I(ip)->i_mode)) {
 		if (XFS_TEST_ERROR(
 		    (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
-		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
+		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
+		    (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
 		    mp, XFS_ERRTAG_IFLUSH_3)) {
 			xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
 				"%s: Bad regular inode %Lu, ptr "PTR_FMT,
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 2389c34c172d..52b297987a75 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -139,6 +139,7 @@ xfs_inode_item_format_data_fork(
 	struct xfs_log_iovec	**vecp)
 {
 	struct xfs_inode	*ip = iip->ili_inode;
+	struct xfs_mount	*mp = ip->i_mount;
 	size_t			data_bytes;
 
 	switch (ip->i_d.di_format) {
@@ -197,7 +198,9 @@ xfs_inode_item_format_data_fork(
 			ASSERT(ip->i_df.if_real_bytes == 0 ||
 			       ip->i_df.if_real_bytes >= data_bytes);
 			ASSERT(ip->i_df.if_u1.if_data != NULL);
-			ASSERT(ip->i_d.di_size > 0);
+			if (!xfs_sb_version_hasinlinedata(&mp->m_sb)) {
+				ASSERT(ip->i_d.di_size > 0);
+			}
 			xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_ILOCAL,
 					ip->i_df.if_u1.if_data, data_bytes);
 			ilf->ilf_dsize = (unsigned)data_bytes;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 55876dd02f0c..aa72966081c5 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -532,7 +532,8 @@ xfs_file_iomap_begin_delay(
 
 	if (unlikely(XFS_TEST_ERROR(
 	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
-	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
+	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&
+	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_LOCAL),
 	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
 		error = -EFSCORRUPTED;
@@ -541,7 +542,7 @@ xfs_file_iomap_begin_delay(
 
 	XFS_STATS_INC(mp, xs_blk_mapw);
 
-	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+	if (!(ifp->if_flags & (XFS_IFINLINE |XFS_IFEXTENTS))) {
 		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
 		if (error)
 			goto out_unlock;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b181b5f57a19..d8828f275d9a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3121,7 +3121,8 @@ xlog_recover_inode_pass2(
 
 	if (unlikely(S_ISREG(ldip->di_mode))) {
 		if ((ldip->di_format != XFS_DINODE_FMT_EXTENTS) &&
-		    (ldip->di_format != XFS_DINODE_FMT_BTREE)) {
+		    (ldip->di_format != XFS_DINODE_FMT_BTREE) &&
+		    (ldip->di_format != XFS_DINODE_FMT_LOCAL)) {
 			XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(3)",
 					 XFS_ERRLEVEL_LOW, mp, ldip,
 					 sizeof(*ldip));
-- 
2.11.0

Re: [PATCH RFC 5/8] xfs: consider the local format inode in misc operations

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:26AM +0800, Shan Hai wrote:
> The local format inode is a legal citizen from now on and consider
> it in misc operations.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c      | 18 ++++++++++--------
>  fs/xfs/libxfs/xfs_inode_buf.c |  6 ------
>  fs/xfs/scrub/inode.c          |  2 +-
>  fs/xfs/xfs_bmap_util.c        | 11 +++++++++--
>  fs/xfs/xfs_inode.c            | 20 ++++++++++++++------
>  fs/xfs/xfs_inode_item.c       |  5 ++++-
>  fs/xfs/xfs_iomap.c            |  5 +++--
>  fs/xfs/xfs_log_recover.c      |  3 ++-
>  8 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index bea6dc254a7d..6b151bd15da7 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -830,11 +830,6 @@ xfs_bmap_local_to_extents(
>  	struct xfs_bmbt_irec rec;
>  	struct xfs_iext_cursor icur;
>  
> -	/*
> -	 * We don't want to deal with the case of keeping inode data inline yet.
> -	 * So sending the data fork of a regular inode is invalid.
> -	 */
> -	ASSERT(!(S_ISREG(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK));
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
>  
> @@ -3840,7 +3835,8 @@ xfs_bmapi_read(
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> -	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),
>  	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
>  		XFS_ERROR_REPORT("xfs_bmapi_read", XFS_ERRLEVEL_LOW, mp);
>  		return -EFSCORRUPTED;
> @@ -3863,7 +3859,7 @@ xfs_bmapi_read(
>  		return 0;
>  	}
>  
> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +	if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
>  		error = xfs_iread_extents(NULL, ip, whichfork);
>  		if (error)
>  			return error;
> @@ -4285,7 +4281,6 @@ xfs_bmapi_write(
>  	       (flags & (XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK)) ==
>  			(XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK));
>  	ASSERT(len > 0);
> -	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
>  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  	ASSERT(!(flags & XFS_BMAPI_REMAP));
>  
> @@ -5244,11 +5239,18 @@ __xfs_bunmapi(
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	if (unlikely(
>  	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> +	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
>  	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>  		XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
>  				 ip->i_mount);
>  		return -EFSCORRUPTED;
>  	}
> +
> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
> +		*rlen = 0;
> +		return 0;
> +	}
> +
>  	mp = ip->i_mount;
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 33dc34655ac3..cb3c4b308137 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -157,7 +157,6 @@ const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
>  	.verify_write = xfs_inode_buf_write_verify,
>  };
>  
> -
>  /*
>   * This routine is called to map an inode to the buffer containing the on-disk
>   * version of the inode.  It returns a pointer to the buffer containing the
> @@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
>  
>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>  	case XFS_DINODE_FMT_LOCAL:
> -		/*
> -		 * no local regular files yet
> -		 */
>  		if (whichfork == XFS_DATA_FORK) {
> -			if (S_ISREG(be16_to_cpu(dip->di_mode)))
> -				return __this_address;
>  			if (be64_to_cpu(dip->di_size) >
>  					XFS_DFORK_SIZE(dip, mp, whichfork))
>  				return __this_address;
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 7a6208505980..1501983dd7aa 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -283,7 +283,7 @@ xfs_scrub_dinode(
>  			xfs_scrub_ino_set_corrupt(sc, ino);
>  		break;
>  	case XFS_DINODE_FMT_LOCAL:
> -		if (!S_ISDIR(mode) && !S_ISLNK(mode))
> +		if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
>  			xfs_scrub_ino_set_corrupt(sc, ino);
>  		break;
>  	case XFS_DINODE_FMT_EXTENTS:
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 83b1e8c6c18f..46f718177fd7 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -760,7 +760,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
>  		return false;
>  
>  	/* If we haven't read in the extent list, then don't do it now. */
> -	if (!(ip->i_df.if_flags & XFS_IFEXTENTS))
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL &&
> +		!(ip->i_df.if_flags & XFS_IFEXTENTS))
>  		return false;
>  
>  	/*
> @@ -768,7 +769,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
>  	 * has delalloc blocks and we are forced to remove them.
>  	 */
>  	if (ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND))
> -		if (!force || ip->i_delayed_blks == 0)
> +		if ((ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) &&
> +			(!force || ip->i_delayed_blks == 0))
>  			return false;
>  
>  	return true;
> @@ -792,6 +794,11 @@ xfs_free_eofblocks(
>  	struct xfs_bmbt_irec	imap;
>  	struct xfs_mount	*mp = ip->i_mount;
>  
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {

Bleh, this indentation is confusing me.

if (foobarblahblabhlbahlabhlabh... &&
    bazcow...)
	dostuff();

or:

if (foobarblahblabhlabhalbhba.. &&
		bazcow...)
	dostuff();

> +		ip->i_delayed_blks = 0;
> +		return 0;
> +	}
>  	/*
>  	 * Figure out if there are any blocks beyond the end
>  	 * of the file.  If not, then there is nothing to do.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5df4de666cc1..78b9790a7cd4 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1563,7 +1563,6 @@ xfs_itruncate_extents_flags(
>  	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
>  
>  	trace_xfs_itruncate_extents_start(ip, new_size);
> -
>  	flags |= xfs_bmapi_aflag(whichfork);
>  
>  	/*
> @@ -1745,9 +1744,15 @@ xfs_inactive_truncate(
>  	ip->i_d.di_size = 0;
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -	error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
> -	if (error)
> -		goto error_trans_cancel;
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +		ASSERT(ip->i_d.di_nextents == 0);
> +		ip->i_delayed_blks = 0;
> +	} else {
> +		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
> +		if (error)
> +			goto error_trans_cancel;
> +	}
>  
>  	ASSERT(ip->i_d.di_nextents == 0);
>  
> @@ -1879,6 +1884,7 @@ xfs_inactive(
>  	if (VFS_I(ip)->i_mode == 0) {
>  		ASSERT(ip->i_df.if_real_bytes == 0);
>  		ASSERT(ip->i_df.if_broot_bytes == 0);
> +		ASSERT(ip->i_delayed_blks == 0);

Why?

>  		return;
>  	}
>  
> @@ -1911,8 +1917,9 @@ xfs_inactive(
>  
>  	if (S_ISREG(VFS_I(ip)->i_mode) &&
>  	    (ip->i_d.di_size != 0 || XFS_ISIZE(ip) != 0 ||
> -	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
> +	     ip->i_d.di_nextents >= 0 || ip->i_delayed_blks >= 0)) {

Why does the test change from > to >= ?

>  		truncate = 1;
> +	}
>  
>  	error = xfs_qm_dqattach(ip);
>  	if (error)
> @@ -3554,7 +3561,8 @@ xfs_iflush_int(
>  	if (S_ISREG(VFS_I(ip)->i_mode)) {
>  		if (XFS_TEST_ERROR(
>  		    (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
> -		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
> +		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
> +		    (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
>  		    mp, XFS_ERRTAG_IFLUSH_3)) {
>  			xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
>  				"%s: Bad regular inode %Lu, ptr "PTR_FMT,
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 2389c34c172d..52b297987a75 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -139,6 +139,7 @@ xfs_inode_item_format_data_fork(
>  	struct xfs_log_iovec	**vecp)
>  {
>  	struct xfs_inode	*ip = iip->ili_inode;
> +	struct xfs_mount	*mp = ip->i_mount;
>  	size_t			data_bytes;
>  
>  	switch (ip->i_d.di_format) {
> @@ -197,7 +198,9 @@ xfs_inode_item_format_data_fork(
>  			ASSERT(ip->i_df.if_real_bytes == 0 ||
>  			       ip->i_df.if_real_bytes >= data_bytes);
>  			ASSERT(ip->i_df.if_u1.if_data != NULL);
> -			ASSERT(ip->i_d.di_size > 0);
> +			if (!xfs_sb_version_hasinlinedata(&mp->m_sb)) {
> +				ASSERT(ip->i_d.di_size > 0);

ASSERT(di_size > 0 || hasinlinedata); ??

> +			}
>  			xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_ILOCAL,
>  					ip->i_df.if_u1.if_data, data_bytes);
>  			ilf->ilf_dsize = (unsigned)data_bytes;
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 55876dd02f0c..aa72966081c5 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -532,7 +532,8 @@ xfs_file_iomap_begin_delay(
>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> -	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
> +	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&
> +	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_LOCAL),
>  	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>  		error = -EFSCORRUPTED;
> @@ -541,7 +542,7 @@ xfs_file_iomap_begin_delay(
>  
>  	XFS_STATS_INC(mp, xs_blk_mapw);
>  
> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +	if (!(ifp->if_flags & (XFS_IFINLINE |XFS_IFEXTENTS))) {
>  		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
>  		if (error)
>  			goto out_unlock;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index b181b5f57a19..d8828f275d9a 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3121,7 +3121,8 @@ xlog_recover_inode_pass2(
>  
>  	if (unlikely(S_ISREG(ldip->di_mode))) {
>  		if ((ldip->di_format != XFS_DINODE_FMT_EXTENTS) &&
> -		    (ldip->di_format != XFS_DINODE_FMT_BTREE)) {
> +		    (ldip->di_format != XFS_DINODE_FMT_BTREE) &&
> +		    (ldip->di_format != XFS_DINODE_FMT_LOCAL)) {
>  			XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(3)",
>  					 XFS_ERRLEVEL_LOW, mp, ldip,
>  					 sizeof(*ldip));
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 5/8] xfs: consider the local format inode in misc operations

[Shan Hai]

On 2018年07月06日 11:40, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:26AM +0800, Shan Hai wrote:
>> The local format inode is a legal citizen from now on and consider
>> it in misc operations.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_bmap.c      | 18 ++++++++++--------
>>   fs/xfs/libxfs/xfs_inode_buf.c |  6 ------
>>   fs/xfs/scrub/inode.c          |  2 +-
>>   fs/xfs/xfs_bmap_util.c        | 11 +++++++++--
>>   fs/xfs/xfs_inode.c            | 20 ++++++++++++++------
>>   fs/xfs/xfs_inode_item.c       |  5 ++++-
>>   fs/xfs/xfs_iomap.c            |  5 +++--
>>   fs/xfs/xfs_log_recover.c      |  3 ++-
>>   8 files changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index bea6dc254a7d..6b151bd15da7 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -830,11 +830,6 @@ xfs_bmap_local_to_extents(
>>   	struct xfs_bmbt_irec rec;
>>   	struct xfs_iext_cursor icur;
>>   
>> -	/*
>> -	 * We don't want to deal with the case of keeping inode data inline yet.
>> -	 * So sending the data fork of a regular inode is invalid.
>> -	 */
>> -	ASSERT(!(S_ISREG(VFS_I(ip)->i_mode) && whichfork == XFS_DATA_FORK));
>>   	ifp = XFS_IFORK_PTR(ip, whichfork);
>>   	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL);
>>   
>> @@ -3840,7 +3835,8 @@ xfs_bmapi_read(
>>   
>>   	if (unlikely(XFS_TEST_ERROR(
>>   	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
>> -	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
>> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
>> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),
>>   	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
>>   		XFS_ERROR_REPORT("xfs_bmapi_read", XFS_ERRLEVEL_LOW, mp);
>>   		return -EFSCORRUPTED;
>> @@ -3863,7 +3859,7 @@ xfs_bmapi_read(
>>   		return 0;
>>   	}
>>   
>> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>> +	if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
>>   		error = xfs_iread_extents(NULL, ip, whichfork);
>>   		if (error)
>>   			return error;
>> @@ -4285,7 +4281,6 @@ xfs_bmapi_write(
>>   	       (flags & (XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK)) ==
>>   			(XFS_BMAPI_CONVERT | XFS_BMAPI_COWFORK));
>>   	ASSERT(len > 0);
>> -	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
>>   	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>   	ASSERT(!(flags & XFS_BMAPI_REMAP));
>>   
>> @@ -5244,11 +5239,18 @@ __xfs_bunmapi(
>>   	ifp = XFS_IFORK_PTR(ip, whichfork);
>>   	if (unlikely(
>>   	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
>> +	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
>>   	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>>   		XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
>>   				 ip->i_mount);
>>   		return -EFSCORRUPTED;
>>   	}
>> +
>> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
>> +		*rlen = 0;
>> +		return 0;
>> +	}
>> +
>>   	mp = ip->i_mount;
>>   	if (XFS_FORCED_SHUTDOWN(mp))
>>   		return -EIO;
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index 33dc34655ac3..cb3c4b308137 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -157,7 +157,6 @@ const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
>>   	.verify_write = xfs_inode_buf_write_verify,
>>   };
>>   
>> -
>>   /*
>>    * This routine is called to map an inode to the buffer containing the on-disk
>>    * version of the inode.  It returns a pointer to the buffer containing the
>> @@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
>>   
>>   	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>>   	case XFS_DINODE_FMT_LOCAL:
>> -		/*
>> -		 * no local regular files yet
>> -		 */
>>   		if (whichfork == XFS_DATA_FORK) {
>> -			if (S_ISREG(be16_to_cpu(dip->di_mode)))
>> -				return __this_address;
>>   			if (be64_to_cpu(dip->di_size) >
>>   					XFS_DFORK_SIZE(dip, mp, whichfork))
>>   				return __this_address;
>> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
>> index 7a6208505980..1501983dd7aa 100644
>> --- a/fs/xfs/scrub/inode.c
>> +++ b/fs/xfs/scrub/inode.c
>> @@ -283,7 +283,7 @@ xfs_scrub_dinode(
>>   			xfs_scrub_ino_set_corrupt(sc, ino);
>>   		break;
>>   	case XFS_DINODE_FMT_LOCAL:
>> -		if (!S_ISDIR(mode) && !S_ISLNK(mode))
>> +		if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
>>   			xfs_scrub_ino_set_corrupt(sc, ino);
>>   		break;
>>   	case XFS_DINODE_FMT_EXTENTS:
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 83b1e8c6c18f..46f718177fd7 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -760,7 +760,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
>>   		return false;
>>   
>>   	/* If we haven't read in the extent list, then don't do it now. */
>> -	if (!(ip->i_df.if_flags & XFS_IFEXTENTS))
>> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL &&
>> +		!(ip->i_df.if_flags & XFS_IFEXTENTS))
>>   		return false;
>>   
>>   	/*
>> @@ -768,7 +769,8 @@ xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
>>   	 * has delalloc blocks and we are forced to remove them.
>>   	 */
>>   	if (ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND))
>> -		if (!force || ip->i_delayed_blks == 0)
>> +		if ((ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) &&
>> +			(!force || ip->i_delayed_blks == 0))
>>   			return false;
>>   
>>   	return true;
>> @@ -792,6 +794,11 @@ xfs_free_eofblocks(
>>   	struct xfs_bmbt_irec	imap;
>>   	struct xfs_mount	*mp = ip->i_mount;
>>   
>> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> Bleh, this indentation is confusing me.
>
> if (foobarblahblabhlbahlabhlabh... &&
>      bazcow...)
> 	dostuff();
>
> or:
>
> if (foobarblahblabhlabhalbhba.. &&
> 		bazcow...)
> 	dostuff();
>
>> +		ip->i_delayed_blks = 0;
>> +		return 0;
>> +	}
>>   	/*
>>   	 * Figure out if there are any blocks beyond the end
>>   	 * of the file.  If not, then there is nothing to do.
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 5df4de666cc1..78b9790a7cd4 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1563,7 +1563,6 @@ xfs_itruncate_extents_flags(
>>   	ASSERT(!XFS_NOT_DQATTACHED(mp, ip));
>>   
>>   	trace_xfs_itruncate_extents_start(ip, new_size);
>> -
>>   	flags |= xfs_bmapi_aflag(whichfork);
>>   
>>   	/*
>> @@ -1745,9 +1744,15 @@ xfs_inactive_truncate(
>>   	ip->i_d.di_size = 0;
>>   	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>>   
>> -	error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
>> -	if (error)
>> -		goto error_trans_cancel;
>> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +		ip->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
>> +		ASSERT(ip->i_d.di_nextents == 0);
>> +		ip->i_delayed_blks = 0;
>> +	} else {
>> +		error = xfs_itruncate_extents(&tp, ip, XFS_DATA_FORK, 0);
>> +		if (error)
>> +			goto error_trans_cancel;
>> +	}
>>   
>>   	ASSERT(ip->i_d.di_nextents == 0);
>>   
>> @@ -1879,6 +1884,7 @@ xfs_inactive(
>>   	if (VFS_I(ip)->i_mode == 0) {
>>   		ASSERT(ip->i_df.if_real_bytes == 0);
>>   		ASSERT(ip->i_df.if_broot_bytes == 0);
>> +		ASSERT(ip->i_delayed_blks == 0);
> Why?

There is no delayed allocation for the existing local format inode so no 
delayed blocks at all,
like below:

1, a local format inode resides on disk
2, open and write data to it, the write_iter copies the data to the data 
fork and returns if
     the data can be inlined, the block allocation is not necessary, so 
there is no delayed
     allocation and the ip->i_delayed_blks == 0
3, unlink the inode after the inline data write, the ip->i_delayed_blks 
should equals 0

>>   		return;
>>   	}
>>   
>> @@ -1911,8 +1917,9 @@ xfs_inactive(
>>   
>>   	if (S_ISREG(VFS_I(ip)->i_mode) &&
>>   	    (ip->i_d.di_size != 0 || XFS_ISIZE(ip) != 0 ||
>> -	     ip->i_d.di_nextents > 0 || ip->i_delayed_blks > 0))
>> +	     ip->i_d.di_nextents >= 0 || ip->i_delayed_blks >= 0)) {
> Why does the test change from > to >= ?

The same reasons as described above, unlinking a local format inode 
would encounter
ip->i_delayed_blks == 0.

But it should be put in the xfs_sb_version_hasinlinedata scope, that's 
totally my fault,
will fix it.

>>   		truncate = 1;
>> +	}
>>   
>>   	error = xfs_qm_dqattach(ip);
>>   	if (error)
>> @@ -3554,7 +3561,8 @@ xfs_iflush_int(
>>   	if (S_ISREG(VFS_I(ip)->i_mode)) {
>>   		if (XFS_TEST_ERROR(
>>   		    (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS) &&
>> -		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE),
>> +		    (ip->i_d.di_format != XFS_DINODE_FMT_BTREE) &&
>> +		    (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL),
>>   		    mp, XFS_ERRTAG_IFLUSH_3)) {
>>   			xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
>>   				"%s: Bad regular inode %Lu, ptr "PTR_FMT,
>> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
>> index 2389c34c172d..52b297987a75 100644
>> --- a/fs/xfs/xfs_inode_item.c
>> +++ b/fs/xfs/xfs_inode_item.c
>> @@ -139,6 +139,7 @@ xfs_inode_item_format_data_fork(
>>   	struct xfs_log_iovec	**vecp)
>>   {
>>   	struct xfs_inode	*ip = iip->ili_inode;
>> +	struct xfs_mount	*mp = ip->i_mount;
>>   	size_t			data_bytes;
>>   
>>   	switch (ip->i_d.di_format) {
>> @@ -197,7 +198,9 @@ xfs_inode_item_format_data_fork(
>>   			ASSERT(ip->i_df.if_real_bytes == 0 ||
>>   			       ip->i_df.if_real_bytes >= data_bytes);
>>   			ASSERT(ip->i_df.if_u1.if_data != NULL);
>> -			ASSERT(ip->i_d.di_size > 0);
>> +			if (!xfs_sb_version_hasinlinedata(&mp->m_sb)) {
>> +				ASSERT(ip->i_d.di_size > 0);
> ASSERT(di_size > 0 || hasinlinedata); ??

Yes, agreed, it's much better.

Thanks
Shan Hai
>> +			}
>>   			xlog_copy_iovec(lv, vecp, XLOG_REG_TYPE_ILOCAL,
>>   					ip->i_df.if_u1.if_data, data_bytes);
>>   			ilf->ilf_dsize = (unsigned)data_bytes;
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 55876dd02f0c..aa72966081c5 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -532,7 +532,8 @@ xfs_file_iomap_begin_delay(
>>   
>>   	if (unlikely(XFS_TEST_ERROR(
>>   	    (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
>> -	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),
>> +	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE &&
>> +	     XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_LOCAL),
>>   	     mp, XFS_ERRTAG_BMAPIFORMAT))) {
>>   		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
>>   		error = -EFSCORRUPTED;
>> @@ -541,7 +542,7 @@ xfs_file_iomap_begin_delay(
>>   
>>   	XFS_STATS_INC(mp, xs_blk_mapw);
>>   
>> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>> +	if (!(ifp->if_flags & (XFS_IFINLINE |XFS_IFEXTENTS))) {
>>   		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
>>   		if (error)
>>   			goto out_unlock;
>> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
>> index b181b5f57a19..d8828f275d9a 100644
>> --- a/fs/xfs/xfs_log_recover.c
>> +++ b/fs/xfs/xfs_log_recover.c
>> @@ -3121,7 +3121,8 @@ xlog_recover_inode_pass2(
>>   
>>   	if (unlikely(S_ISREG(ldip->di_mode))) {
>>   		if ((ldip->di_format != XFS_DINODE_FMT_EXTENTS) &&
>> -		    (ldip->di_format != XFS_DINODE_FMT_BTREE)) {
>> +		    (ldip->di_format != XFS_DINODE_FMT_BTREE) &&
>> +		    (ldip->di_format != XFS_DINODE_FMT_LOCAL)) {
>>   			XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(3)",
>>   					 XFS_ERRLEVEL_LOW, mp, ldip,
>>   					 sizeof(*ldip));
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 5/8] xfs: consider the local format inode in misc operations

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 11:12:26AM +0800, Shan Hai wrote:
> The local format inode is a legal citizen from now on and consider
> it in misc operations.

This probably wants to go towards the front of the series, and split
into multiple patches with better changelogs for each.

>  
>  	if (unlikely(XFS_TEST_ERROR(
>  	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> -	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),

This are all the three possible values.  It is kinda confusing to read
the checks this way as we alreayd checked for a valid fork type in
the inode read verifies.

Also this seems to be in xfs_bmapi_read, for which reading allowing
inline format forks seems wrong to me.

> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +	if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
>  		error = xfs_iread_extents(NULL, ip, whichfork);
>  		if (error)
>  			return error;

I think we need to clean this up better.  We probably want an
xfs_inode_has_extents helper, or even hide the check inside
xfs_iread_extents itself.  Also I wonder if the above is read, the
XFS_IFEXTENTS flag has always meant that we have the current extent list
in memory, which should always be the case for a fork in inline format.

> @@ -5244,11 +5239,18 @@ __xfs_bunmapi(
>  	ifp = XFS_IFORK_PTR(ip, whichfork);
>  	if (unlikely(
>  	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> +	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
>  	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>  		XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
>  				 ip->i_mount);
>  		return -EFSCORRUPTED;
>  	}
> +
> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
> +		*rlen = 0;
> +		return 0;
> +	}

I don't think we should ever call bunmapi for an inline format fork.

>  };
>  
> -
>  /*

Spurious whitespace change.

>   * This routine is called to map an inode to the buffer containing the on-disk
>   * version of the inode.  It returns a pointer to the buffer containing the
> @@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
>  
>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>  	case XFS_DINODE_FMT_LOCAL:
> -		/*
> -		 * no local regular files yet
> -		 */
>  		if (whichfork == XFS_DATA_FORK) {
> -			if (S_ISREG(be16_to_cpu(dip->di_mode)))
> -				return __this_address;

I think you need to check the new feature bit here and only allow the
inline regular case if the feature bit it set.

> @@ -283,7 +283,7 @@ xfs_scrub_dinode(
>  			xfs_scrub_ino_set_corrupt(sc, ino);
>  		break;
>  	case XFS_DINODE_FMT_LOCAL:
> -		if (!S_ISDIR(mode) && !S_ISLNK(mode))
> +		if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
>  			xfs_scrub_ino_set_corrupt(sc, ino);
>  		break;

Same here.

Re: [PATCH RFC 5/8] xfs: consider the local format inode in misc operations

[Shan Hai]

On 2018年07月08日 23:51, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 11:12:26AM +0800, Shan Hai wrote:
>> The local format inode is a legal citizen from now on and consider
>> it in misc operations.
> This probably wants to go towards the front of the series, and split
> into multiple patches with better changelogs for each.
>
>>   
>>   	if (unlikely(XFS_TEST_ERROR(
>>   	    (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
>> -	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
>> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
>> +	     XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),
> This are all the three possible values.  It is kinda confusing to read
> the checks this way as we alreayd checked for a valid fork type in
> the inode read verifies.
>
> Also this seems to be in xfs_bmapi_read, for which reading allowing
> inline format forks seems wrong to me.

Agreed.

>> -	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>> +	if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
>>   		error = xfs_iread_extents(NULL, ip, whichfork);
>>   		if (error)
>>   			return error;
> I think we need to clean this up better.  We probably want an
> xfs_inode_has_extents helper, or even hide the check inside
> xfs_iread_extents itself.  Also I wonder if the above is read, the
> XFS_IFEXTENTS flag has always meant that we have the current extent list
> in memory, which should always be the case for a fork in inline format.
>

Yes, you are right, seems the above 2 checks are left over from my code 
cleanup before
sending it out, my apology for wasting your time like this,  really sorry.

>> @@ -5244,11 +5239,18 @@ __xfs_bunmapi(
>>   	ifp = XFS_IFORK_PTR(ip, whichfork);
>>   	if (unlikely(
>>   	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
>> +	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
>>   	    XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
>>   		XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
>>   				 ip->i_mount);
>>   		return -EFSCORRUPTED;
>>   	}
>> +
>> +	if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
>> +		*rlen = 0;
>> +		return 0;
>> +	}
> I don't think we should ever call bunmapi for an inline format fork.
>
>>   };
>>   
>> -
>>   /*
> Spurious whitespace change.
>
>>    * This routine is called to map an inode to the buffer containing the on-disk
>>    * version of the inode.  It returns a pointer to the buffer containing the
>> @@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
>>   
>>   	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>>   	case XFS_DINODE_FMT_LOCAL:
>> -		/*
>> -		 * no local regular files yet
>> -		 */
>>   		if (whichfork == XFS_DATA_FORK) {
>> -			if (S_ISREG(be16_to_cpu(dip->di_mode)))
>> -				return __this_address;
> I think you need to check the new feature bit here and only allow the
> inline regular case if the feature bit it set.

OK, I will do that.

>> @@ -283,7 +283,7 @@ xfs_scrub_dinode(
>>   			xfs_scrub_ino_set_corrupt(sc, ino);
>>   		break;
>>   	case XFS_DINODE_FMT_LOCAL:
>> -		if (!S_ISDIR(mode) && !S_ISLNK(mode))
>> +		if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
>>   			xfs_scrub_ino_set_corrupt(sc, ino);
>>   		break;
> Same here.

Ditto.

Thanks
Shan Hai

[PATCH RFC 6/8] xfs: fix imbalanced locking

[Shan Hai]

The XFS_ILOCK_SHARED type lock is held in the xfs_ioc_fsgetxattr
but does not released when the inode is in local format, fix it by
eleasing the lock on the local format inode.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/xfs_ioctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0ef5ece5634c..246562615ccd 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -907,7 +907,10 @@ xfs_ioc_fsgetxattr(
 	} else {
 		if (ip->i_df.if_flags & XFS_IFEXTENTS)
 			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
-		else
+		else if (ip->i_df.if_flags & XFS_IFINLINE) {
+			xfs_iunlock(ip, XFS_ILOCK_SHARED);
+			return 0;
+		} else
 			fa.fsx_nextents = ip->i_d.di_nextents;
 	}
 	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-- 
2.11.0

Re: [PATCH RFC 6/8] xfs: fix imbalanced locking

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 11:12:27AM +0800, Shan Hai wrote:
> The XFS_ILOCK_SHARED type lock is held in the xfs_ioc_fsgetxattr
> but does not released when the inode is in local format, fix it by
> eleasing the lock on the local format inode.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0ef5ece5634c..246562615ccd 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -907,7 +907,10 @@ xfs_ioc_fsgetxattr(
>  	} else {
>  		if (ip->i_df.if_flags & XFS_IFEXTENTS)
>  			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
> -		else
> +		else if (ip->i_df.if_flags & XFS_IFINLINE) {
> +			xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +			return 0;
> +		} else
>  			fa.fsx_nextents = ip->i_d.di_nextents;
>  	}
>  	xfs_iunlock(ip, XFS_ILOCK_SHARED);

This looks odd, and the changelog confuses even more.

Please avoid the early return, which means we miss the copy_to_user
of the fa struct.  It seems to me like this should be something like:

		if (ip->i_df.if_flags & XFS_IFEXTENTS)
                        fa.fsx_nextents = xfs_iext_count(&ip->i_df);
		esle if (ip->i_df.if_flags & XFS_IFINLINE)
			fa.fsx_nextents = 0;
                else
			fa.fsx_nextents = ip->i_d.di_nextents;

Re: [PATCH RFC 6/8] xfs: fix imbalanced locking

[Shan Hai]

On 2018年07月08日 23:53, Christoph Hellwig wrote:
> On Fri, Jul 06, 2018 at 11:12:27AM +0800, Shan Hai wrote:
>> The XFS_ILOCK_SHARED type lock is held in the xfs_ioc_fsgetxattr
>> but does not released when the inode is in local format, fix it by
>> eleasing the lock on the local format inode.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_ioctl.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 0ef5ece5634c..246562615ccd 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -907,7 +907,10 @@ xfs_ioc_fsgetxattr(
>>   	} else {
>>   		if (ip->i_df.if_flags & XFS_IFEXTENTS)
>>   			fa.fsx_nextents = xfs_iext_count(&ip->i_df);
>> -		else
>> +		else if (ip->i_df.if_flags & XFS_IFINLINE) {
>> +			xfs_iunlock(ip, XFS_ILOCK_SHARED);
>> +			return 0;
>> +		} else
>>   			fa.fsx_nextents = ip->i_d.di_nextents;
>>   	}
>>   	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> This looks odd, and the changelog confuses even more.
>
> Please avoid the early return, which means we miss the copy_to_user
> of the fa struct.  It seems to me like this should be something like:
>
> 		if (ip->i_df.if_flags & XFS_IFEXTENTS)
>                          fa.fsx_nextents = xfs_iext_count(&ip->i_df);
> 		esle if (ip->i_df.if_flags & XFS_IFINLINE)
> 			fa.fsx_nextents = 0;
>                  else
> 			fa.fsx_nextents = ip->i_d.di_nextents;


Agreed, I will double check this, thanks for the review.

Thanks
Shan Hai

[PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Shan Hai]

Return non-zero blocks for inline data even though the inode has
no external blocks, otherwise the "ls -ls" would show zero blocks
occupied by the file.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/xfs_iops.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0fa29f39d658..63be1355a473 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -500,8 +500,14 @@ xfs_vn_getattr(
 	stat->atime = inode->i_atime;
 	stat->mtime = inode->i_mtime;
 	stat->ctime = inode->i_ctime;
-	stat->blocks =
+
+	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
+		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
+			stat->blocks = BTOBB(XFS_ISIZE(ip));
+	} else {
+		stat->blocks =
 		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
+	}
 
 	if (ip->i_d.di_version == 3) {
 		if (request_mask & STATX_BTIME) {
-- 
2.11.0

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
> Return non-zero blocks for inline data even though the inode has
> no external blocks, otherwise the "ls -ls" would show zero blocks
> occupied by the file.

Does this match the behaviour of existing file systems with inline
data?  What do btrfs, ext4 or gfs2 report?

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Carlos Maiolino]

On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
> Return non-zero blocks for inline data even though the inode has
> no external blocks, otherwise the "ls -ls" would show zero blocks
> occupied by the file.
> 

Is there any issue you ran into while leaving inodes with zero blocks allocated?
Inodes should actually report the real amount of allocated blocks, not fake it.
Inodes with inlined data should actually report 0 blocks, otherwise, many
applications which actually relies on the amount of allocated blocks for each
file will misbehave.

> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_iops.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 0fa29f39d658..63be1355a473 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -500,8 +500,14 @@ xfs_vn_getattr(
>  	stat->atime = inode->i_atime;
>  	stat->mtime = inode->i_mtime;
>  	stat->ctime = inode->i_ctime;
> -	stat->blocks =
> +
> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> +			stat->blocks = BTOBB(XFS_ISIZE(ip));
> +	} else {
> +		stat->blocks =
>  		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
> +	}
>  
>  	if (ip->i_d.di_version == 3) {
>  		if (request_mask & STATX_BTIME) {
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Shan Hai]

On 2018年07月11日 21:08, Carlos Maiolino wrote:
> On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
>> Return non-zero blocks for inline data even though the inode has
>> no external blocks, otherwise the "ls -ls" would show zero blocks
>> occupied by the file.
>>
> Is there any issue you ran into while leaving inodes with zero blocks allocated?
> Inodes should actually report the real amount of allocated blocks, not fake it.
> Inodes with inlined data should actually report 0 blocks, otherwise, many
> applications which actually relies on the amount of allocated blocks for each
> file will misbehave.
>

Man ls(1) reads:

-s, --size
     print the allocated size of each file, in blocks

So the 'ls -ls' would report 0 blocks when the data is inlined, a file 
holds data
but it consumes 0 blocks, how is it possible :), this patch fixes this 
problem.

Thanks
Shan Hai
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_iops.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index 0fa29f39d658..63be1355a473 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -500,8 +500,14 @@ xfs_vn_getattr(
>>   	stat->atime = inode->i_atime;
>>   	stat->mtime = inode->i_mtime;
>>   	stat->ctime = inode->i_ctime;
>> -	stat->blocks =
>> +
>> +	if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>> +		XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>> +			stat->blocks = BTOBB(XFS_ISIZE(ip));
>> +	} else {
>> +		stat->blocks =
>>   		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>> +	}
>>   
>>   	if (ip->i_d.di_version == 3) {
>>   		if (request_mask & STATX_BTIME) {
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Shan Hai]

On 2018年07月12日 09:03, Shan Hai wrote:
>
>
> On 2018年07月11日 21:08, Carlos Maiolino wrote:
>> On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
>>> Return non-zero blocks for inline data even though the inode has
>>> no external blocks, otherwise the "ls -ls" would show zero blocks
>>> occupied by the file.
>>>
>> Is there any issue you ran into while leaving inodes with zero blocks 
>> allocated?
>> Inodes should actually report the real amount of allocated blocks, 
>> not fake it.
>> Inodes with inlined data should actually report 0 blocks, otherwise, 
>> many
>> applications which actually relies on the amount of allocated blocks 
>> for each
>> file will misbehave.
>>
>
> Man ls(1) reads:
>
> -s, --size
>     print the allocated size of each file, in blocks
>
> So the 'ls -ls' would report 0 blocks when the data is inlined, a file 
> holds data
> but it consumes 0 blocks, how is it possible :), this patch fixes this 
> problem.
>

This patch is inspired by the
upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for 
inline data),
please refer it for details.

Thanks
Shan Hai

> Thanks
> Shan Hai
>>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>>> ---
>>>   fs/xfs/xfs_iops.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>> index 0fa29f39d658..63be1355a473 100644
>>> --- a/fs/xfs/xfs_iops.c
>>> +++ b/fs/xfs/xfs_iops.c
>>> @@ -500,8 +500,14 @@ xfs_vn_getattr(
>>>       stat->atime = inode->i_atime;
>>>       stat->mtime = inode->i_mtime;
>>>       stat->ctime = inode->i_ctime;
>>> -    stat->blocks =
>>> +
>>> +    if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>>> +        XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>>> +            stat->blocks = BTOBB(XFS_ISIZE(ip));
>>> +    } else {
>>> +        stat->blocks =
>>>           XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>>> +    }
>>>         if (ip->i_d.di_version == 3) {
>>>           if (request_mask & STATX_BTIME) {
>>> -- 
>>> 2.11.0
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Darrick J. Wong]

On Thu, Jul 12, 2018 at 09:13:46AM +0800, Shan Hai wrote:
> 
> 
> On 2018年07月12日 09:03, Shan Hai wrote:
> > 
> > 
> > On 2018年07月11日 21:08, Carlos Maiolino wrote:
> > > On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
> > > > Return non-zero blocks for inline data even though the inode has
> > > > no external blocks, otherwise the "ls -ls" would show zero blocks
> > > > occupied by the file.
> > > > 
> > > Is there any issue you ran into while leaving inodes with zero
> > > blocks allocated?
> > > Inodes should actually report the real amount of allocated blocks,
> > > not fake it.
> > > Inodes with inlined data should actually report 0 blocks, otherwise,
> > > many
> > > applications which actually relies on the amount of allocated blocks
> > > for each
> > > file will misbehave.
> > > 
> > 
> > Man ls(1) reads:
> > 
> > -s, --size
> >     print the allocated size of each file, in blocks
> > 
> > So the 'ls -ls' would report 0 blocks when the data is inlined, a file
> > holds data
> > but it consumes 0 blocks, how is it possible :), this patch fixes this
> > problem.
> > 
> 
> This patch is inspired by the
> upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for inline
> data),
> please refer it for details.

The fact that we're following precedent set by ext4 is worth mentioning
in the commit message.

--D

> Thanks
> Shan Hai
> 
> > Thanks
> > Shan Hai
> > > > Signed-off-by: Shan Hai <shan.hai@oracle.com>
> > > > ---
> > > >   fs/xfs/xfs_iops.c | 8 +++++++-
> > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index 0fa29f39d658..63be1355a473 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -500,8 +500,14 @@ xfs_vn_getattr(
> > > >       stat->atime = inode->i_atime;
> > > >       stat->mtime = inode->i_mtime;
> > > >       stat->ctime = inode->i_ctime;
> > > > -    stat->blocks =
> > > > +
> > > > +    if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
> > > > +        XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
> > > > +            stat->blocks = BTOBB(XFS_ISIZE(ip));
> > > > +    } else {
> > > > +        stat->blocks =
> > > >           XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
> > > > +    }
> > > >         if (ip->i_d.di_version == 3) {
> > > >           if (request_mask & STATX_BTIME) {
> > > > -- 
> > > > 2.11.0
> > > > 
> > > > -- 
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 
> > -- 
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Shan Hai]

On 2018年07月12日 09:31, Darrick J. Wong wrote:
> On Thu, Jul 12, 2018 at 09:13:46AM +0800, Shan Hai wrote:
>>
>> On 2018年07月12日 09:03, Shan Hai wrote:
>>>
>>> On 2018年07月11日 21:08, Carlos Maiolino wrote:
>>>> On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
>>>>> Return non-zero blocks for inline data even though the inode has
>>>>> no external blocks, otherwise the "ls -ls" would show zero blocks
>>>>> occupied by the file.
>>>>>
>>>> Is there any issue you ran into while leaving inodes with zero
>>>> blocks allocated?
>>>> Inodes should actually report the real amount of allocated blocks,
>>>> not fake it.
>>>> Inodes with inlined data should actually report 0 blocks, otherwise,
>>>> many
>>>> applications which actually relies on the amount of allocated blocks
>>>> for each
>>>> file will misbehave.
>>>>
>>> Man ls(1) reads:
>>>
>>> -s, --size
>>>      print the allocated size of each file, in blocks
>>>
>>> So the 'ls -ls' would report 0 blocks when the data is inlined, a file
>>> holds data
>>> but it consumes 0 blocks, how is it possible :), this patch fixes this
>>> problem.
>>>
>> This patch is inspired by the
>> upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for inline
>> data),
>> please refer it for details.
> The fact that we're following precedent set by ext4 is worth mentioning
> in the commit message.

OK, I will include it in the next version of the patch.

Thanks
Shan Hai
> --D
>
>> Thanks
>> Shan Hai
>>
>>> Thanks
>>> Shan Hai
>>>>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>>>>> ---
>>>>>    fs/xfs/xfs_iops.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>>>> index 0fa29f39d658..63be1355a473 100644
>>>>> --- a/fs/xfs/xfs_iops.c
>>>>> +++ b/fs/xfs/xfs_iops.c
>>>>> @@ -500,8 +500,14 @@ xfs_vn_getattr(
>>>>>        stat->atime = inode->i_atime;
>>>>>        stat->mtime = inode->i_mtime;
>>>>>        stat->ctime = inode->i_ctime;
>>>>> -    stat->blocks =
>>>>> +
>>>>> +    if (xfs_sb_version_hasinlinedata(&mp->m_sb) &&
>>>>> +        XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) == XFS_DINODE_FMT_LOCAL) {
>>>>> +            stat->blocks = BTOBB(XFS_ISIZE(ip));
>>>>> +    } else {
>>>>> +        stat->blocks =
>>>>>            XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>>>>> +    }
>>>>>          if (ip->i_d.di_version == 3) {
>>>>>            if (request_mask & STATX_BTIME) {
>>>>> -- 
>>>>> 2.11.0
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Carlos Maiolino]

On Thu, Jul 12, 2018 at 09:46:20AM +0800, Shan Hai wrote:
> 
> 
> On 2018年07月12日 09:31, Darrick J. Wong wrote:
> > On Thu, Jul 12, 2018 at 09:13:46AM +0800, Shan Hai wrote:
> > > 
> > > On 2018年07月12日 09:03, Shan Hai wrote:
> > > > 
> > > > On 2018年07月11日 21:08, Carlos Maiolino wrote:
> > > > > On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
> > > > > > Return non-zero blocks for inline data even though the inode has
> > > > > > no external blocks, otherwise the "ls -ls" would show zero blocks
> > > > > > occupied by the file.
> > > > > > 
> > > > > Is there any issue you ran into while leaving inodes with zero
> > > > > blocks allocated?
> > > > > Inodes should actually report the real amount of allocated blocks,
> > > > > not fake it.
> > > > > Inodes with inlined data should actually report 0 blocks, otherwise,
> > > > > many
> > > > > applications which actually relies on the amount of allocated blocks
> > > > > for each
> > > > > file will misbehave.
> > > > > 
> > > > Man ls(1) reads:
> > > > 
> > > > -s, --size
> > > >      print the allocated size of each file, in blocks
> > > > 
> > > > So the 'ls -ls' would report 0 blocks when the data is inlined, a file
> > > > holds data
> > > > but it consumes 0 blocks, how is it possible :),

It is possible because the file doesn't consume data blocks at all.

> > > This patch is inspired by the
> > > upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for inline
> > > data),
> > > please refer it for details.
> > The fact that we're following precedent set by ext4 is worth mentioning
> > in the commit message.

The fact another filesystem use this trick, doesn't necessarily means it's
correct. Ext4 added it to workaround a issue with tar, which actually skip zero
blocks files. I honestly think it is wrong, we are working around a user space
problem, which is wrongly assuming a 0-block file is empty.

A quick search led me to this thread from tar project:

http://www.mail-archive.com/bug-tar@gnu.org/msg04209.html

which well, from that time, they were already aware that always assuming a
zero-block file is empty, was not safe.



Reporting a used block in a file that is storing data inlined in the inode is
prone for space usage accounting error IMO.

Suppose you have a 4K block-size filesystem, you create 100k files into it with
inlined data.

So now you have 100k files reporting to be using a single FSB, wich converts
into a bit less than 400MB.

How will you make sure the accounting is correct? You can't really mark any
blocks used in the filesystem, so, tools like `df` (or any other tool based on
statfs() ), will report these 400MB as free.

At the same time, tools like `ls`, `du`, etc which relies on per-file stat, will
report these 400MB used.


I do see advantages on reporting a single block use, but IMO, it will cause more
confusion than good.

Anyway, as I said, my opinion only, I do really think reporting a single block
used by inlined files is wrong, and if we are going to do that, we should at
least properly document this is being done to workaround user space issues,
while, in the meantime, it might create others.

Cheers.

-- 
Carlos

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Shan Hai]

On 2018年07月12日 17:08, Carlos Maiolino wrote:
> On Thu, Jul 12, 2018 at 09:46:20AM +0800, Shan Hai wrote:
>>
>> On 2018年07月12日 09:31, Darrick J. Wong wrote:
>>> On Thu, Jul 12, 2018 at 09:13:46AM +0800, Shan Hai wrote:
>>>> On 2018年07月12日 09:03, Shan Hai wrote:
>>>>> On 2018年07月11日 21:08, Carlos Maiolino wrote:
>>>>>> On Fri, Jul 06, 2018 at 11:12:28AM +0800, Shan Hai wrote:
>>>>>>> Return non-zero blocks for inline data even though the inode has
>>>>>>> no external blocks, otherwise the "ls -ls" would show zero blocks
>>>>>>> occupied by the file.
>>>>>>>
>>>>>> Is there any issue you ran into while leaving inodes with zero
>>>>>> blocks allocated?
>>>>>> Inodes should actually report the real amount of allocated blocks,
>>>>>> not fake it.
>>>>>> Inodes with inlined data should actually report 0 blocks, otherwise,
>>>>>> many
>>>>>> applications which actually relies on the amount of allocated blocks
>>>>>> for each
>>>>>> file will misbehave.
>>>>>>
>>>>> Man ls(1) reads:
>>>>>
>>>>> -s, --size
>>>>>       print the allocated size of each file, in blocks
>>>>>
>>>>> So the 'ls -ls' would report 0 blocks when the data is inlined, a file
>>>>> holds data
>>>>> but it consumes 0 blocks, how is it possible :),
> It is possible because the file doesn't consume data blocks at all.
>
>>>> This patch is inspired by the
>>>> upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for inline
>>>> data),
>>>> please refer it for details.
>>> The fact that we're following precedent set by ext4 is worth mentioning
>>> in the commit message.
> The fact another filesystem use this trick, doesn't necessarily means it's
> correct. Ext4 added it to workaround a issue with tar, which actually skip zero
> blocks files. I honestly think it is wrong, we are working around a user space
> problem, which is wrongly assuming a 0-block file is empty.

The assumption which deemed 0 block files as empty holds true until the 
kernel broke the
rules and introduced inline data feature, so the user space should not 
be blamed in my opinion.

> A quick search led me to this thread from tar project:
>
> http://www.mail-archive.com/bug-tar@gnu.org/msg04209.html
>
> which well, from that time, they were already aware that always assuming a
> zero-block file is empty, was not safe.
>
>
>
> Reporting a used block in a file that is storing data inlined in the inode is
> prone for space usage accounting error IMO.
>
> Suppose you have a 4K block-size filesystem, you create 100k files into it with
> inlined data.
>
> So now you have 100k files reporting to be using a single FSB, wich converts
> into a bit less than 400MB.
>
> How will you make sure the accounting is correct? You can't really mark any
> blocks used in the filesystem, so, tools like `df` (or any other tool based on
> statfs() ), will report these 400MB as free.
>
> At the same time, tools like `ls`, `du`, etc which relies on per-file stat, will
> report these 400MB used.
>
>
> I do see advantages on reporting a single block use, but IMO, it will cause more
> confusion than good.
>
> Anyway, as I said, my opinion only, I do really think reporting a single block
> used by inlined files is wrong, and if we are going to do that, we should at
> least properly document this is being done to workaround user space issues,
> while, in the meantime, it might create others.

Faking block usage count is not quite correct one but the sad fact is 
that there are applications
out there which are designed and implemented on the assumption that 
non-zero length files
consume at least a block, so I don't think breaking the user space 
suddenly by reporting 0 blocks
for inline data is not a correct solution either.

I will put a big fat note in inline comment to emphasize this issue.
Any other better ideas?

Thanks
Shan Hai
> Cheers.
>

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Carlos Maiolino]

> > > > > > > Inodes with inlined data should actually report 0 blocks, otherwise,
> > > > > > > many
> > > > > > > applications which actually relies on the amount of allocated blocks
> > > > > > > for each
> > > > > > > file will misbehave.
> > > > > > > 
> > > > > > Man ls(1) reads:
> > > > > > 
> > > > > > -s, --size
> > > > > >       print the allocated size of each file, in blocks
> > > > > > 
> > > > > > So the 'ls -ls' would report 0 blocks when the data is inlined, a file
> > > > > > holds data
> > > > > > but it consumes 0 blocks, how is it possible :),
> > It is possible because the file doesn't consume data blocks at all.
> > 
> > > > > This patch is inspired by the
> > > > > upstream commit 9206c561554c9 (ext4: return non-zero st_blocks for inline
> > > > > data),
> > > > > please refer it for details.
> > > > The fact that we're following precedent set by ext4 is worth mentioning
> > > > in the commit message.
> > The fact another filesystem use this trick, doesn't necessarily means it's
> > correct. Ext4 added it to workaround a issue with tar, which actually skip zero
> > blocks files. I honestly think it is wrong, we are working around a user space
> > problem, which is wrongly assuming a 0-block file is empty.
> 
> The assumption which deemed 0 block files as empty holds true until the
> kernel broke the
> rules and introduced inline data feature, so the user space should not be
> blamed in my opinion.

Did we? I couldn't find any interface, design, or whatever saying that userspace
can safely assume a zero block file don't have data there and can be ignored,
in fact, the link I posted previously already shows how tar people were not sure
if they should assume a zero block file could be ignored. So, in fact, we didn't
break anything, because as far as I know, nobody and no standard said an
existing zero block file could be safely ignored, assuming it contains no data
at all. Such thing might exist, but I'm not aware of.

> 
> > A quick search led me to this thread from tar project:
> > 
> > http://www.mail-archive.com/bug-tar@gnu.org/msg04209.html
> > 
> > 
> > Anyway, as I said, my opinion only, I do really think reporting a single block
> > used by inlined files is wrong, and if we are going to do that, we should at
> > least properly document this is being done to workaround user space issues,
> > while, in the meantime, it might create others.
> 
> Faking block usage count is not quite correct one but the sad fact is that
> there are applications
> out there which are designed and implemented on the assumption that non-zero
> length files
> consume at least a block, so I don't think breaking the user space suddenly
> by reporting 0 blocks
> for inline data is not a correct solution either.
> 

I understand, and yeah, we fell into some kind of chicken-egg problem, I still
do think though there are not that much applications out there assuming
zero block files are empty, and working around bad coded application which make
assumptions they are not supposed to make, is as bad as breaking applications
for not following standards, which will end up punishing applications which do
not make assumptions they should.

The biggest drawback I can see by reporting 0 blocks allocated for inline files,
might be while estimating space needed for a copy, or a backup for example,
where, if the target filesystem does not support inlined data, 0 blocks files
will occupy at least a block on the target filesystem, but the same holds true
when source and target filesystems have different block sizes.

Anyway, that's all the arguments I have against faking block usage for inlined
files, but the final decision is not mine :)

Cheers

-- 
Carlos

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Christoph Hellwig]

On Fri, Jul 13, 2018 at 02:39:55PM +0200, Carlos Maiolino wrote:
> Did we? I couldn't find any interface, design, or whatever saying that userspace
> can safely assume a zero block file don't have data there and can be ignored,
> in fact, the link I posted previously already shows how tar people were not sure
> if they should assume a zero block file could be ignored. So, in fact, we didn't
> break anything, because as far as I know, nobody and no standard said an
> existing zero block file could be safely ignored, assuming it contains no data
> at all. Such thing might exist, but I'm not aware of.

You are right that there is no interface.  But breaking existing
userspace simply isn't an option either, especially when it could
lead to data loss and corruption.

Re: [PATCH RFC 7/8] xfs: return non-zero blocks for inline data

[Carlos Maiolino]

On Tue, Jul 17, 2018 at 06:57:41AM -0700, Christoph Hellwig wrote:
> On Fri, Jul 13, 2018 at 02:39:55PM +0200, Carlos Maiolino wrote:
> > Did we? I couldn't find any interface, design, or whatever saying that userspace
> > can safely assume a zero block file don't have data there and can be ignored,
> > in fact, the link I posted previously already shows how tar people were not sure
> > if they should assume a zero block file could be ignored. So, in fact, we didn't
> > break anything, because as far as I know, nobody and no standard said an
> > existing zero block file could be safely ignored, assuming it contains no data
> > at all. Such thing might exist, but I'm not aware of.
> 
> You are right that there is no interface.  But breaking existing
> userspace simply isn't an option either, especially when it could
> lead to data loss and corruption.

Fair enough.

-- 
Carlos

[PATCH RFC 8/8] xfs: skip local format inode for reflinking

[Shan Hai]

The local format inode holds the data inline in its data fork and
has no extents at all, so skip the inode for reflinking.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 fs/xfs/xfs_reflink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 592fb2071a03..dfb3a85ec0e6 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1507,6 +1507,12 @@ xfs_reflink_inode_has_shared_extents(
 	int				error;
 
 	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+
+	if (ifp->if_flags & XFS_IFINLINE) {
+		*has_shared = false;
+		return 0;
+	}
+
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, XFS_DATA_FORK);
 		if (error)
-- 
2.11.0

Re: [PATCH RFC 8/8] xfs: skip local format inode for reflinking

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:29AM +0800, Shan Hai wrote:
> The local format inode holds the data inline in its data fork and
> has no extents at all, so skip the inode for reflinking.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  fs/xfs/xfs_reflink.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 592fb2071a03..dfb3a85ec0e6 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1507,6 +1507,12 @@ xfs_reflink_inode_has_shared_extents(
>  	int				error;
>  
>  	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +
> +	if (ifp->if_flags & XFS_IFINLINE) {
> +		*has_shared = false;
> +		return 0;
> +	}

What happens if someone tries to reflink and either src or dest are an
inline file?

--D

> +
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(tp, ip, XFS_DATA_FORK);
>  		if (error)
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 8/8] xfs: skip local format inode for reflinking

[Shan Hai]

On 2018年07月06日 11:26, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:29AM +0800, Shan Hai wrote:
>> The local format inode holds the data inline in its data fork and
>> has no extents at all, so skip the inode for reflinking.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>   fs/xfs/xfs_reflink.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>> index 592fb2071a03..dfb3a85ec0e6 100644
>> --- a/fs/xfs/xfs_reflink.c
>> +++ b/fs/xfs/xfs_reflink.c
>> @@ -1507,6 +1507,12 @@ xfs_reflink_inode_has_shared_extents(
>>   	int				error;
>>   
>>   	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>> +
>> +	if (ifp->if_flags & XFS_IFINLINE) {
>> +		*has_shared = false;
>> +		return 0;
>> +	}
> What happens if someone tries to reflink and either src or dest are an
> inline file?

I have to re-check this, thanks for the suggestion.

Thanks
Shan Hai
> --D
>
>> +
>>   	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>>   		error = xfs_iread_extents(tp, ip, XFS_DATA_FORK);
>>   		if (error)
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 8/8] xfs: skip local format inode for reflinking

[Christoph Hellwig]

On Thu, Jul 05, 2018 at 08:26:53PM -0700, Darrick J. Wong wrote:
> What happens if someone tries to reflink and either src or dest are an
> inline file?

Very good question.  I'd also like to see how btrfs and gfs2
handle that case, as it would be good if all Linux file systems
behave the same way.

[PATCH RFC 1/1] xfsprogs: add inode inline data support

[Shan Hai]

Add a new mkfs command line option to enable the inode inline data
feature. The mkfs set a bit in the superblock to notify the kernel
that the data inlining is enabled, there is no extra options for
mount.

Signed-off-by: Shan Hai <shan.hai@oracle.com>
---
 growfs/xfs_growfs.c | 14 +++++++++-----
 libxfs/xfs_format.h |  4 +++-
 libxfs/xfs_fs.h     |  1 +
 mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index 366176b7..a370bf9d 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -60,13 +60,14 @@ report_info(
 	int		finobt_enabled,
 	int		spinodes,
 	int		rmapbt_enabled,
-	int		reflink_enabled)
+	int		reflink_enabled,
+	int		inlinedata_enabled)
 {
 	printf(_(
 	    "meta-data=%-22s isize=%-6u agcount=%u, agsize=%u blks\n"
 	    "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
 	    "         =%-22s crc=%-8u finobt=%u spinodes=%u rmapbt=%u\n"
-	    "         =%-22s reflink=%u\n"
+	    "         =%-22s reflink=%u inline=%u\n"
 	    "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
 	    "         =%-22s sunit=%-6u swidth=%u blks\n"
 	    "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
@@ -77,7 +78,7 @@ report_info(
 		mntpoint, geo.inodesize, geo.agcount, geo.agblocks,
 		"", geo.sectsize, attrversion, projid32bit,
 		"", crcs_enabled, finobt_enabled, spinodes, rmapbt_enabled,
-		"", reflink_enabled,
+		"", reflink_enabled, inlinedata_enabled,
 		"", geo.blocksize, (unsigned long long)geo.datablocks,
 			geo.imaxpct,
 		"", geo.sunit, geo.swidth,
@@ -133,6 +134,7 @@ main(int argc, char **argv)
 	int			spinodes;
 	int			rmapbt_enabled;
 	int			reflink_enabled;
+	int			inlinedata_enabled;
 	char			rpath[PATH_MAX];
 
 	progname = basename(argv[0]);
@@ -266,12 +268,14 @@ main(int argc, char **argv)
 	spinodes = geo.flags & XFS_FSOP_GEOM_FLAGS_SPINODES ? 1 : 0;
 	rmapbt_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_RMAPBT ? 1 : 0;
 	reflink_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_REFLINK ? 1 : 0;
+	inlinedata_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_INLINE ? 1 : 0;
 	if (nflag) {
 		report_info(geo, datadev, isint, logdev, rtdev,
 				lazycount, dirversion, logversion,
 				attrversion, projid32bit, crcs_enabled, ci,
 				ftype_enabled, finobt_enabled, spinodes,
-				rmapbt_enabled, reflink_enabled);
+				rmapbt_enabled, reflink_enabled,
+				inlinedata_enabled);
 		exit(0);
 	}
 
@@ -310,7 +314,7 @@ main(int argc, char **argv)
 			lazycount, dirversion, logversion,
 			attrversion, projid32bit, crcs_enabled, ci, ftype_enabled,
 			finobt_enabled, spinodes, rmapbt_enabled,
-			reflink_enabled);
+			reflink_enabled, inlinedata_enabled);
 
 	ddsize = xi.dsize;
 	dlsize = ( xi.logBBsize? xi.logBBsize :
diff --git a/libxfs/xfs_format.h b/libxfs/xfs_format.h
index 48548933..2a765d12 100644
--- a/libxfs/xfs_format.h
+++ b/libxfs/xfs_format.h
@@ -478,10 +478,12 @@ xfs_sb_has_ro_compat_feature(
 #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
 #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
 #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
+#define XFS_SB_FEAT_INCOMPAT_INLINEDATA	(1 << 3)	/* inline inode data */
 #define XFS_SB_FEAT_INCOMPAT_ALL \
 		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
 		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
-		 XFS_SB_FEAT_INCOMPAT_META_UUID)
+		 XFS_SB_FEAT_INCOMPAT_META_UUID| \
+		 XFS_SB_FEAT_INCOMPAT_INLINEDATA)
 
 #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
 static inline bool
diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
index 86a379f6..40737fb9 100644
--- a/libxfs/xfs_fs.h
+++ b/libxfs/xfs_fs.h
@@ -243,6 +243,7 @@ typedef struct xfs_fsop_resblks {
 #define XFS_FSOP_GEOM_FLAGS_SPINODES	0x40000	/* sparse inode chunks	*/
 #define XFS_FSOP_GEOM_FLAGS_RMAPBT	0x80000	/* reverse mapping btree */
 #define XFS_FSOP_GEOM_FLAGS_REFLINK	0x100000 /* files can share blocks */
+#define XFS_FSOP_GEOM_FLAGS_INLINE	0x200000 /* inline data into inode */
 
 /*
  * Minimum and maximum sizes need for growth checks.
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 78d0ce5d..b321761b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -82,6 +82,7 @@ enum {
 	I_ATTR,
 	I_PROJID32BIT,
 	I_SPINODES,
+	I_INLINE,
 	I_MAX_OPTS,
 };
 
@@ -394,6 +395,7 @@ struct opt_params iopts = {
 		[I_ATTR] = "attr",
 		[I_PROJID32BIT] = "projid32bit",
 		[I_SPINODES] = "sparse",
+		[I_INLINE] = "inline",
 	},
 	.subopt_params = {
 		{ .index = I_ALIGN,
@@ -442,6 +444,12 @@ struct opt_params iopts = {
 		  .maxval = 1,
 		  .defaultval = 1,
 		},
+		{ .index = I_INLINE,
+		  .conflicts = { { NULL, LAST_CONFLICT } },
+		  .minval = 0,
+		  .maxval = 1,
+		  .defaultval = 0,
+		},
 	},
 };
 
@@ -743,6 +751,7 @@ struct sb_feat_args {
 	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
 	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
 	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
+	bool	inlinedata;		/* XFS_SB_FEAT_INCOMPAT_INLINEDATA */
 	bool	nodalign;
 	bool	nortalign;
 };
@@ -871,7 +880,7 @@ usage( void )
 			    sectsize=num\n\
 /* force overwrite */	[-f]\n\
 /* inode size */	[-i log=n|perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
-			    projid32bit=0|1,sparse=0|1]\n\
+			    projid32bit=0|1,sparse=0|1,inline=0|1]\n\
 /* no discard */	[-K]\n\
 /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
 			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\
@@ -1506,6 +1515,9 @@ inode_opts_parser(
 	case I_SPINODES:
 		cli->sb_feat.spinodes = getnum(value, opts, subopt);
 		break;
+	case I_INLINE:
+		cli->sb_feat.inlinedata = getnum(value, opts, subopt);
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -2903,6 +2915,8 @@ sb_set_features(
 		sbp->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_SPINODES;
 	}
 
+	if (fp->inlinedata)
+		sbp->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_INLINEDATA;
 }
 
 /*
@@ -3184,7 +3198,7 @@ print_mkfs_cfg(
 	printf(_(
 "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
 "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
-"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
+"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u, inline=%u\n"
 "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
 "         =%-22s sunit=%-6u swidth=%u blks\n"
 "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
@@ -3195,7 +3209,7 @@ print_mkfs_cfg(
 			(long long)cfg->agsize,
 		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
 		"", fp->crcs_enabled, fp->finobt, fp->spinodes, fp->rmapbt,
-			fp->reflink,
+			fp->reflink, fp->inlinedata,
 		"", cfg->blocksize, (long long)cfg->dblocks, cfg->imaxpct,
 		"", cfg->dsunit, cfg->dswidth,
 		fp->dir_version, cfg->dirblocksize, fp->nci, fp->dirftype,
-- 
2.11.0

Re: [PATCH RFC 1/1] xfsprogs: add inode inline data support

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:30AM +0800, Shan Hai wrote:
> Add a new mkfs command line option to enable the inode inline data
> feature. The mkfs set a bit in the superblock to notify the kernel
> that the data inlining is enabled, there is no extra options for
> mount.
> 
> Signed-off-by: Shan Hai <shan.hai@oracle.com>
> ---
>  growfs/xfs_growfs.c | 14 +++++++++-----
>  libxfs/xfs_format.h |  4 +++-
>  libxfs/xfs_fs.h     |  1 +
>  mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index 366176b7..a370bf9d 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -60,13 +60,14 @@ report_info(
>  	int		finobt_enabled,
>  	int		spinodes,
>  	int		rmapbt_enabled,
> -	int		reflink_enabled)
> +	int		reflink_enabled,
> +	int		inlinedata_enabled)
>  {
>  	printf(_(
>  	    "meta-data=%-22s isize=%-6u agcount=%u, agsize=%u blks\n"
>  	    "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
>  	    "         =%-22s crc=%-8u finobt=%u spinodes=%u rmapbt=%u\n"
> -	    "         =%-22s reflink=%u\n"
> +	    "         =%-22s reflink=%u inline=%u\n"

I coulda sworn we refactored this into a library function...

Do xfs_db or xfs_repair require any changes?

Did xfstests have anything exciting to say about this feature?

--D

>  	    "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
>  	    "         =%-22s sunit=%-6u swidth=%u blks\n"
>  	    "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> @@ -77,7 +78,7 @@ report_info(
>  		mntpoint, geo.inodesize, geo.agcount, geo.agblocks,
>  		"", geo.sectsize, attrversion, projid32bit,
>  		"", crcs_enabled, finobt_enabled, spinodes, rmapbt_enabled,
> -		"", reflink_enabled,
> +		"", reflink_enabled, inlinedata_enabled,
>  		"", geo.blocksize, (unsigned long long)geo.datablocks,
>  			geo.imaxpct,
>  		"", geo.sunit, geo.swidth,
> @@ -133,6 +134,7 @@ main(int argc, char **argv)
>  	int			spinodes;
>  	int			rmapbt_enabled;
>  	int			reflink_enabled;
> +	int			inlinedata_enabled;
>  	char			rpath[PATH_MAX];
>  
>  	progname = basename(argv[0]);
> @@ -266,12 +268,14 @@ main(int argc, char **argv)
>  	spinodes = geo.flags & XFS_FSOP_GEOM_FLAGS_SPINODES ? 1 : 0;
>  	rmapbt_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_RMAPBT ? 1 : 0;
>  	reflink_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_REFLINK ? 1 : 0;
> +	inlinedata_enabled = geo.flags & XFS_FSOP_GEOM_FLAGS_INLINE ? 1 : 0;
>  	if (nflag) {
>  		report_info(geo, datadev, isint, logdev, rtdev,
>  				lazycount, dirversion, logversion,
>  				attrversion, projid32bit, crcs_enabled, ci,
>  				ftype_enabled, finobt_enabled, spinodes,
> -				rmapbt_enabled, reflink_enabled);
> +				rmapbt_enabled, reflink_enabled,
> +				inlinedata_enabled);
>  		exit(0);
>  	}
>  
> @@ -310,7 +314,7 @@ main(int argc, char **argv)
>  			lazycount, dirversion, logversion,
>  			attrversion, projid32bit, crcs_enabled, ci, ftype_enabled,
>  			finobt_enabled, spinodes, rmapbt_enabled,
> -			reflink_enabled);
> +			reflink_enabled, inlinedata_enabled);
>  
>  	ddsize = xi.dsize;
>  	dlsize = ( xi.logBBsize? xi.logBBsize :
> diff --git a/libxfs/xfs_format.h b/libxfs/xfs_format.h
> index 48548933..2a765d12 100644
> --- a/libxfs/xfs_format.h
> +++ b/libxfs/xfs_format.h
> @@ -478,10 +478,12 @@ xfs_sb_has_ro_compat_feature(
>  #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
>  #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
>  #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
> +#define XFS_SB_FEAT_INCOMPAT_INLINEDATA	(1 << 3)	/* inline inode data */
>  #define XFS_SB_FEAT_INCOMPAT_ALL \
>  		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
>  		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
> -		 XFS_SB_FEAT_INCOMPAT_META_UUID)
> +		 XFS_SB_FEAT_INCOMPAT_META_UUID| \
> +		 XFS_SB_FEAT_INCOMPAT_INLINEDATA)
>  
>  #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
>  static inline bool
> diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> index 86a379f6..40737fb9 100644
> --- a/libxfs/xfs_fs.h
> +++ b/libxfs/xfs_fs.h
> @@ -243,6 +243,7 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_FSOP_GEOM_FLAGS_SPINODES	0x40000	/* sparse inode chunks	*/
>  #define XFS_FSOP_GEOM_FLAGS_RMAPBT	0x80000	/* reverse mapping btree */
>  #define XFS_FSOP_GEOM_FLAGS_REFLINK	0x100000 /* files can share blocks */
> +#define XFS_FSOP_GEOM_FLAGS_INLINE	0x200000 /* inline data into inode */
>  
>  /*
>   * Minimum and maximum sizes need for growth checks.
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 78d0ce5d..b321761b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -82,6 +82,7 @@ enum {
>  	I_ATTR,
>  	I_PROJID32BIT,
>  	I_SPINODES,
> +	I_INLINE,
>  	I_MAX_OPTS,
>  };
>  
> @@ -394,6 +395,7 @@ struct opt_params iopts = {
>  		[I_ATTR] = "attr",
>  		[I_PROJID32BIT] = "projid32bit",
>  		[I_SPINODES] = "sparse",
> +		[I_INLINE] = "inline",
>  	},
>  	.subopt_params = {
>  		{ .index = I_ALIGN,
> @@ -442,6 +444,12 @@ struct opt_params iopts = {
>  		  .maxval = 1,
>  		  .defaultval = 1,
>  		},
> +		{ .index = I_INLINE,
> +		  .conflicts = { { NULL, LAST_CONFLICT } },
> +		  .minval = 0,
> +		  .maxval = 1,
> +		  .defaultval = 0,
> +		},
>  	},
>  };
>  
> @@ -743,6 +751,7 @@ struct sb_feat_args {
>  	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
>  	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
>  	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
> +	bool	inlinedata;		/* XFS_SB_FEAT_INCOMPAT_INLINEDATA */
>  	bool	nodalign;
>  	bool	nortalign;
>  };
> @@ -871,7 +880,7 @@ usage( void )
>  			    sectsize=num\n\
>  /* force overwrite */	[-f]\n\
>  /* inode size */	[-i log=n|perblock=n|size=num,maxpct=n,attr=0|1|2,\n\
> -			    projid32bit=0|1,sparse=0|1]\n\
> +			    projid32bit=0|1,sparse=0|1,inline=0|1]\n\
>  /* no discard */	[-K]\n\
>  /* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n\n\
>  			    sunit=value|su=num,sectsize=num,lazy-count=0|1]\n\
> @@ -1506,6 +1515,9 @@ inode_opts_parser(
>  	case I_SPINODES:
>  		cli->sb_feat.spinodes = getnum(value, opts, subopt);
>  		break;
> +	case I_INLINE:
> +		cli->sb_feat.inlinedata = getnum(value, opts, subopt);
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -2903,6 +2915,8 @@ sb_set_features(
>  		sbp->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_SPINODES;
>  	}
>  
> +	if (fp->inlinedata)
> +		sbp->sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_INLINEDATA;
>  }
>  
>  /*
> @@ -3184,7 +3198,7 @@ print_mkfs_cfg(
>  	printf(_(
>  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
>  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> -"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> +"         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u, inline=%u\n"
>  "data     =%-22s bsize=%-6u blocks=%llu, imaxpct=%u\n"
>  "         =%-22s sunit=%-6u swidth=%u blks\n"
>  "naming   =version %-14u bsize=%-6u ascii-ci=%d ftype=%d\n"
> @@ -3195,7 +3209,7 @@ print_mkfs_cfg(
>  			(long long)cfg->agsize,
>  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
>  		"", fp->crcs_enabled, fp->finobt, fp->spinodes, fp->rmapbt,
> -			fp->reflink,
> +			fp->reflink, fp->inlinedata,
>  		"", cfg->blocksize, (long long)cfg->dblocks, cfg->imaxpct,
>  		"", cfg->dsunit, cfg->dswidth,
>  		fp->dir_version, cfg->dirblocksize, fp->nci, fp->dirftype,
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/1] xfsprogs: add inode inline data support

[Eric Sandeen]

On 7/5/18 10:35 PM, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:30AM +0800, Shan Hai wrote:
>> Add a new mkfs command line option to enable the inode inline data
>> feature. The mkfs set a bit in the superblock to notify the kernel
>> that the data inlining is enabled, there is no extra options for
>> mount.
>>
>> Signed-off-by: Shan Hai <shan.hai@oracle.com>
>> ---
>>  growfs/xfs_growfs.c | 14 +++++++++-----
>>  libxfs/xfs_format.h |  4 +++-
>>  libxfs/xfs_fs.h     |  1 +
>>  mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
>>  4 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
>> index 366176b7..a370bf9d 100644
>> --- a/growfs/xfs_growfs.c
>> +++ b/growfs/xfs_growfs.c
>> @@ -60,13 +60,14 @@ report_info(
>>  	int		finobt_enabled,
>>  	int		spinodes,
>>  	int		rmapbt_enabled,
>> -	int		reflink_enabled)
>> +	int		reflink_enabled,
>> +	int		inlinedata_enabled)
>>  {
>>  	printf(_(
>>  	    "meta-data=%-22s isize=%-6u agcount=%u, agsize=%u blks\n"
>>  	    "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
>>  	    "         =%-22s crc=%-8u finobt=%u spinodes=%u rmapbt=%u\n"
>> -	    "         =%-22s reflink=%u\n"
>> +	    "         =%-22s reflink=%u inline=%u\n"
> I coulda sworn we refactored this into a library function...

We did, and it's in 4.17.0 now.

On the next version please rebase this to whatever's in the xfsprogs for-next
branch at the time, that's where all the action is.  :)

Thanks,
-Eric

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Darrick J. Wong]

On Fri, Jul 06, 2018 at 11:12:21AM +0800, Shan Hai wrote:
> 
> This series implements xfs inode data inlining feature.
> 
> Refered below link during development:
> https://marc.info/?l=linux-xfs&m=120493585731509&w=2
> 
> 
> How it works:
> - the data inlining happens at:
>   write_iter: for DIO/DAX write
>   writeback: for buffered write
> - extents to local format conversion is done in writeback but not in write_iter
> - local to extents format conversion is done in the write_iter and writeback
> - log the whole inode (core/data) on extents to local conversion
> - add a new mkfs.xfs option to enable data inline feature
> 
> How to test:
> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdX
> mount /dev/sdX mnt_pnt
> 
> Test results:
> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdb
> mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdd
> mount /dev/sdb /mnt/sdb
> mount /dev/sdd /mnt/sdd
> 
> Copy the linux v4.18-rc3 source code to /mnt/sdb and /mnt/sdd respectively:
> Filesystem                        Size  Used Avail Use% Mounted on
> /dev/sdb                           10G  2.4G  7.7G  24% /mnt/sdb
> /dev/sdd                           10G  3.1G  6.9G  31% /mnt/sdd
> 
> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdc
> mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sde
> mount /dev/sdc /mnt/sdc
> mount /dev/sde /mnt/sde
> 
> Build the v4.18-rc3 kernel on /dev/sdc and /dev/sde respectively:
> make x86_64_defconfig
> 
> Filesystem                        Size  Used Avail Use% Mounted on
> /dev/sdc                           10G  689M  9.4G   7% /mnt/sdc
> /dev/sde                           10G  694M  9.4G   7% /mnt/sde

Not much of a savings... but I'll give this a preliminary review anyway.

Also, I am probably going to merge hch's bufferhead killing series for
4.19, which will rip right through a lot of this. :)

--D

> 
> Kernel part:
> 0001-xfs-introduce-inline-data-superblock-feature-bit.patch
> 0002-xfs-introduce-extents-to-local-conversion-helper.patch
> 0003-xfs-convert-inode-from-extents-to-local-format.patch
> 0004-xfs-implement-inline-data-read-write-code.patch
> 0005-xfs-consider-the-local-format-inode-in-misc-operatio.patch
> 0006-xfs-fix-imbalanced-locking.patch
> 0007-xfs-return-non-zero-blocks-for-inline-data.patch
> 0008-xfs-skip-local-format-inode-for-reflinking.patch
> 
>  fs/xfs/libxfs/xfs_bmap.c      |  78 +++++++++++++++++++++++++++++++-----
>  fs/xfs/libxfs/xfs_bmap.h      |   4 ++
>  fs/xfs/libxfs/xfs_format.h    |  10 ++++-
>  fs/xfs/libxfs/xfs_fs.h        |   1 +
>  fs/xfs/libxfs/xfs_inode_buf.c |   6 ---
>  fs/xfs/libxfs/xfs_sb.c        |   2 +
>  fs/xfs/scrub/inode.c          |   2 +-
>  fs/xfs/xfs_aops.c             |  60 ++++++++++++++++++++++++++++
>  fs/xfs/xfs_bmap_util.c        |  11 +++++-
>  fs/xfs/xfs_file.c             | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_inode.c            |  20 +++++++---
>  fs/xfs/xfs_inode_item.c       |   5 ++-
>  fs/xfs/xfs_ioctl.c            |   5 ++-
>  fs/xfs/xfs_iomap.c            |   5 ++-
>  fs/xfs/xfs_iops.c             |   8 +++-
>  fs/xfs/xfs_log_recover.c      |   3 +-
>  fs/xfs/xfs_reflink.c          |   6 +++
>  17 files changed, 437 insertions(+), 33 deletions(-)
> 
> xfsprogs part:
> 0001-xfsprogs-add-inode-inline-data-support.patch 
> 
>  growfs/xfs_growfs.c | 14 +++++++++-----
>  libxfs/xfs_format.h |  4 +++-
>  libxfs/xfs_fs.h     |  1 +
>  mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
>  4 files changed, 30 insertions(+), 9 deletions(-)
> 
> 
> Thanks
> Shan Hai
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Shan Hai]

On 2018年07月06日 11:51, Darrick J. Wong wrote:
> On Fri, Jul 06, 2018 at 11:12:21AM +0800, Shan Hai wrote:
>> This series implements xfs inode data inlining feature.
>>
>> Refered below link during development:
>> https://marc.info/?l=linux-xfs&m=120493585731509&w=2
>>
>>
>> How it works:
>> - the data inlining happens at:
>>    write_iter: for DIO/DAX write
>>    writeback: for buffered write
>> - extents to local format conversion is done in writeback but not in write_iter
>> - local to extents format conversion is done in the write_iter and writeback
>> - log the whole inode (core/data) on extents to local conversion
>> - add a new mkfs.xfs option to enable data inline feature
>>
>> How to test:
>> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdX
>> mount /dev/sdX mnt_pnt
>>
>> Test results:
>> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdb
>> mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdd
>> mount /dev/sdb /mnt/sdb
>> mount /dev/sdd /mnt/sdd
>>
>> Copy the linux v4.18-rc3 source code to /mnt/sdb and /mnt/sdd respectively:
>> Filesystem                        Size  Used Avail Use% Mounted on
>> /dev/sdb                           10G  2.4G  7.7G  24% /mnt/sdb
>> /dev/sdd                           10G  3.1G  6.9G  31% /mnt/sdd

An observable savings here.

>> mkfs.xfs -f -i size=2048,inline=1 -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sdc
>> mkfs.xfs -f -i size=2048          -m crc=1,finobt=1,rmapbt=1,reflink=1 /dev/sde
>> mount /dev/sdc /mnt/sdc
>> mount /dev/sde /mnt/sde
>>
>> Build the v4.18-rc3 kernel on /dev/sdc and /dev/sde respectively:
>> make x86_64_defconfig
>>
>> Filesystem                        Size  Used Avail Use% Mounted on
>> /dev/sdc                           10G  689M  9.4G   7% /mnt/sdc
>> /dev/sde                           10G  694M  9.4G   7% /mnt/sde
> Not much of a savings... but I'll give this a preliminary review anyway.
>
> Also, I am probably going to merge hch's bufferhead killing series for
> 4.19, which will rip right through a lot of this. :)

That would be fine, thanks in advance for the review anyway :)

Thanks
Shan Hai
> --D
>
>> Kernel part:
>> 0001-xfs-introduce-inline-data-superblock-feature-bit.patch
>> 0002-xfs-introduce-extents-to-local-conversion-helper.patch
>> 0003-xfs-convert-inode-from-extents-to-local-format.patch
>> 0004-xfs-implement-inline-data-read-write-code.patch
>> 0005-xfs-consider-the-local-format-inode-in-misc-operatio.patch
>> 0006-xfs-fix-imbalanced-locking.patch
>> 0007-xfs-return-non-zero-blocks-for-inline-data.patch
>> 0008-xfs-skip-local-format-inode-for-reflinking.patch
>>
>>   fs/xfs/libxfs/xfs_bmap.c      |  78 +++++++++++++++++++++++++++++++-----
>>   fs/xfs/libxfs/xfs_bmap.h      |   4 ++
>>   fs/xfs/libxfs/xfs_format.h    |  10 ++++-
>>   fs/xfs/libxfs/xfs_fs.h        |   1 +
>>   fs/xfs/libxfs/xfs_inode_buf.c |   6 ---
>>   fs/xfs/libxfs/xfs_sb.c        |   2 +
>>   fs/xfs/scrub/inode.c          |   2 +-
>>   fs/xfs/xfs_aops.c             |  60 ++++++++++++++++++++++++++++
>>   fs/xfs/xfs_bmap_util.c        |  11 +++++-
>>   fs/xfs/xfs_file.c             | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   fs/xfs/xfs_inode.c            |  20 +++++++---
>>   fs/xfs/xfs_inode_item.c       |   5 ++-
>>   fs/xfs/xfs_ioctl.c            |   5 ++-
>>   fs/xfs/xfs_iomap.c            |   5 ++-
>>   fs/xfs/xfs_iops.c             |   8 +++-
>>   fs/xfs/xfs_log_recover.c      |   3 +-
>>   fs/xfs/xfs_reflink.c          |   6 +++
>>   17 files changed, 437 insertions(+), 33 deletions(-)
>>
>> xfsprogs part:
>> 0001-xfsprogs-add-inode-inline-data-support.patch
>>
>>   growfs/xfs_growfs.c | 14 +++++++++-----
>>   libxfs/xfs_format.h |  4 +++-
>>   libxfs/xfs_fs.h     |  1 +
>>   mkfs/xfs_mkfs.c     | 20 +++++++++++++++++---
>>   4 files changed, 30 insertions(+), 9 deletions(-)
>>
>>
>> Thanks
>> Shan Hai
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Dave Chinner]

On Fri, Jul 06, 2018 at 11:12:21AM +0800, Shan Hai wrote:
> 
> This series implements xfs inode data inlining feature.

Cool. I was actually thinking about this earlier this week. :)

> Refered below link during development:
> https://marc.info/?l=linux-xfs&m=120493585731509&w=2
> 
> 
> How it works:
> - the data inlining happens at:
>   write_iter: for DIO/DAX write
>   writeback: for buffered write
> - extents to local format conversion is done in writeback but not in write_iter
> - local to extents format conversion is done in the write_iter and writeback

The problem with the way local to extents conversion is imlpemented
in this patchset is that we do not synchronise data writeback with
journal commits and log recovery.  The local to extents conversion
appears to be done like so:

----
writeback arrives in local format, data in extent cached page
convert inode to extent format
allocate new block
commit allocation and inode format conversion
submit data IO to newly allocated block.
----

This works when nothing goes wrong, but it's not failsafe. The
problem is that we've logged the inode data fork conversion before
we've completed writing the data to the new block. IOWs, we can end
up in the state where the active journal recoery window does not contain
the data in the inode and the data has not yet reached the new
allocated block on disk, either.

If we crash in this window, we lose the data that was in the inode -
log recovery finishes with the inode in extent form pointing to
uninitialised data blocks. i.e. not only is it a data loss event,
it's also a security issue because it exposes stale data.

This has been the unsolved problem from all previous attempts to
implement inline data in XFS. I actually outlined this problem and
ways to solve it in the mailing list post linked above, but it
doesn't appear that either mechanism was implemented in this
patchset.

However, unlike that post from 2008, we now have infrastructure that
can be used to solve to this problem: the data path copy-on-write
mechanism.  The COW mechanism is essentially a generic
implementation of the "Method 1: use an intent/done transaction
pair" solution I describe in the above post.

This mechanism solves the above problem by storing the newly
allocated block in the in-memory COW fork and doesn't modify the
data fork until after the data write IO completes. IOWs, we do
allocation before the write IO, and do the data fork and BMBT
manipulation after the IO completes. i.e. we do the local->extent
data fork modification at IO completion using the extent that was
stored in the in memory COW fork.

Hence I think the inline data write path needs to piggy back on the
iomap COW path that we use for writing to shared extents if the
write would cause a data fork format change.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Shan Hai]

On 2018年07月06日 13:42, Dave Chinner wrote:
> On Fri, Jul 06, 2018 at 11:12:21AM +0800, Shan Hai wrote:
>> This series implements xfs inode data inlining feature.
> Cool. I was actually thinking about this earlier this week. :)
>

Yes, this is a nice feature, and thanks for your previous suggestions in 
the below link,
which saved my time a lot :)

>> Refered below link during development:
>> https://marc.info/?l=linux-xfs&m=120493585731509&w=2
>>
>>
>> How it works:
>> - the data inlining happens at:
>>    write_iter: for DIO/DAX write
>>    writeback: for buffered write
>> - extents to local format conversion is done in writeback but not in write_iter
>> - local to extents format conversion is done in the write_iter and writeback
> The problem with the way local to extents conversion is imlpemented
> in this patchset is that we do not synchronise data writeback with
> journal commits and log recovery.  The local to extents conversion
> appears to be done like so:
>
> ----
> writeback arrives in local format, data in extent cached page
> convert inode to extent format
> allocate new block
> commit allocation and inode format conversion
> submit data IO to newly allocated block.
> ----
>
> This works when nothing goes wrong, but it's not failsafe. The
> problem is that we've logged the inode data fork conversion before
> we've completed writing the data to the new block. IOWs, we can end
> up in the state where the active journal recoery window does not contain
> the data in the inode and the data has not yet reached the new
> allocated block on disk, either.
>
> If we crash in this window, we lose the data that was in the inode -
> log recovery finishes with the inode in extent form pointing to
> uninitialised data blocks. i.e. not only is it a data loss event,
> it's also a security issue because it exposes stale data.
>
> This has been the unsolved problem from all previous attempts to
> implement inline data in XFS. I actually outlined this problem and
> ways to solve it in the mailing list post linked above, but it
> doesn't appear that either mechanism was implemented in this
> patchset.
>
> However, unlike that post from 2008, we now have infrastructure that
> can be used to solve to this problem: the data path copy-on-write
> mechanism.  The COW mechanism is essentially a generic
> implementation of the "Method 1: use an intent/done transaction
> pair" solution I describe in the above post.
>
> This mechanism solves the above problem by storing the newly
> allocated block in the in-memory COW fork and doesn't modify the
> data fork until after the data write IO completes. IOWs, we do
> allocation before the write IO, and do the data fork and BMBT
> manipulation after the IO completes. i.e. we do the local->extent
> data fork modification at IO completion using the extent that was
> stored in the in memory COW fork.

Yes, this can fix the race totally, I will try to implement it in the 
subsequent series.

Or how about below? It's similar to what we did in the xfs_setattr_size, 
but the sync
write in this situation is ugly I know :)

----
write arrives at local inode
allocate a page and insert it into page cache, copy the data from data 
fork to the page
filemap_write_and_wait_range
xfs_trans_roll
convert inode to extent format
allocate a new block
commit allocation and inode format conversion
----

Thanks
Shan Hai
> Hence I think the inline data write path needs to piggy back on the
> iomap COW path that we use for writing to shared extents if the
> write would cause a data fork format change.
>
> Cheers,
>
> Dave.

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Shan Hai]

On 2018年07月06日 14:39, Shan Hai wrote:
>
>
> On 2018年07月06日 13:42, Dave Chinner wrote:
>> On Fri, Jul 06, 2018 at 11:12:21AM +0800, Shan Hai wrote:
>>> This series implements xfs inode data inlining feature.
>> Cool. I was actually thinking about this earlier this week. :)
>>
>
> Yes, this is a nice feature, and thanks for your previous suggestions 
> in the below link,
> which saved my time a lot :)
>
>>> Refered below link during development:
>>> https://marc.info/?l=linux-xfs&m=120493585731509&w=2
>>>
>>>
>>> How it works:
>>> - the data inlining happens at:
>>>    write_iter: for DIO/DAX write
>>>    writeback: for buffered write
>>> - extents to local format conversion is done in writeback but not in 
>>> write_iter
>>> - local to extents format conversion is done in the write_iter and 
>>> writeback
>> The problem with the way local to extents conversion is imlpemented
>> in this patchset is that we do not synchronise data writeback with
>> journal commits and log recovery.  The local to extents conversion
>> appears to be done like so:
>>
>> ----
>> writeback arrives in local format, data in extent cached page
>> convert inode to extent format
>> allocate new block
>> commit allocation and inode format conversion
>> submit data IO to newly allocated block.
>> ----
>>
>> This works when nothing goes wrong, but it's not failsafe. The
>> problem is that we've logged the inode data fork conversion before
>> we've completed writing the data to the new block. IOWs, we can end
>> up in the state where the active journal recoery window does not contain
>> the data in the inode and the data has not yet reached the new
>> allocated block on disk, either.
>>
>> If we crash in this window, we lose the data that was in the inode -
>> log recovery finishes with the inode in extent form pointing to
>> uninitialised data blocks. i.e. not only is it a data loss event,
>> it's also a security issue because it exposes stale data.
>>
>> This has been the unsolved problem from all previous attempts to
>> implement inline data in XFS. I actually outlined this problem and
>> ways to solve it in the mailing list post linked above, but it
>> doesn't appear that either mechanism was implemented in this
>> patchset.
>>
>> However, unlike that post from 2008, we now have infrastructure that
>> can be used to solve to this problem: the data path copy-on-write
>> mechanism.  The COW mechanism is essentially a generic
>> implementation of the "Method 1: use an intent/done transaction
>> pair" solution I describe in the above post.
>>
>> This mechanism solves the above problem by storing the newly
>> allocated block in the in-memory COW fork and doesn't modify the
>> data fork until after the data write IO completes. IOWs, we do
>> allocation before the write IO, and do the data fork and BMBT
>> manipulation after the IO completes. i.e. we do the local->extent
>> data fork modification at IO completion using the extent that was
>> stored in the in memory COW fork.
>
> Yes, this can fix the race totally, I will try to implement it in the 
> subsequent series.
>
> Or how about below? It's similar to what we did in the 
> xfs_setattr_size, but the sync
> write in this situation is ugly I know :)
>
> ----
> write arrives at local inode
> allocate a page and insert it into page cache, copy the data from data 
> fork to the page
> filemap_write_and_wait_range

Oh no, please ignore this, it would never can work, I will try the data 
path COW idea.

Thanks
Shan Hai
>
> xfs_trans_roll
> convert inode to extent format
> allocate a new block
> commit allocation and inode format conversion
> ----
>
> Thanks
> Shan Hai
>> Hence I think the inline data write path needs to piggy back on the
>> iomap COW path that we use for writing to shared extents if the
>> write would cause a data fork format change.
>>
>> Cheers,
>>
>> Dave.
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/8] xfs: introduce inode data inline feature

[Christoph Hellwig]

On Fri, Jul 06, 2018 at 03:42:50PM +1000, Dave Chinner wrote:
> This mechanism solves the above problem by storing the newly
> allocated block in the in-memory COW fork and doesn't modify the
> data fork until after the data write IO completes. IOWs, we do
> allocation before the write IO, and do the data fork and BMBT
> manipulation after the IO completes. i.e. we do the local->extent
> data fork modification at IO completion using the extent that was
> stored in the in memory COW fork.
> 
> Hence I think the inline data write path needs to piggy back on the
> iomap COW path that we use for writing to shared extents if the
> write would cause a data fork format change.

I'm not even sure we need the cow infrastructure for that, but yes,
we need the format conversion to be driven from the I/O completion
handler for buffer I/O writeback, and for direct I/O (including AIO)
as well.