[PATCH] debugfs: Add inline data feature for symlink.

[PATCH] debugfs: Add inline data feature for symlink.

[Pu Hou]

Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.

Signed-off-by: Pu Hou <houpu.hp@alibaba-inc.com>
---
 lib/ext2fs/ext2fs.h      |  3 +++
 lib/ext2fs/inline_data.c | 26 +++++++++++++++++++++++
 lib/ext2fs/symlink.c     | 55 +++++++++++++++++++++++++++++++++++-------------
 3 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 7812956..8268d96 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1401,6 +1401,9 @@ extern errcode_t ext2fs_inline_data_get(ext2_filsys fs, ext2_ino_t ino,
 extern errcode_t ext2fs_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
 					struct ext2_inode *inode,
 					void *buf, size_t size);
+extern errcode_t ext2fs_symlink_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
+					struct ext2_inode *inode,
+					void *buf, size_t size);
 
 /* inode.c */
 extern errcode_t ext2fs_create_inode_cache(ext2_filsys fs,
diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
index 19248a0..ed82a32 100644
--- a/lib/ext2fs/inline_data.c
+++ b/lib/ext2fs/inline_data.c
@@ -585,6 +585,32 @@ errcode_t ext2fs_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
 	return ext2fs_inline_data_ea_set(&data);
 }
 
+errcode_t ext2fs_symlink_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
+				 struct ext2_inode *inode,
+				 void *buf, size_t size)
+{
+	struct ext2_inode inode_buf;
+	struct ext2_inline_data data;
+	errcode_t retval;
+	size_t free_ea_size, existing_size, free_inode_size;
+
+	if (!inode) {
+		retval = ext2fs_read_inode(fs, ino, &inode_buf);
+		if (retval)
+			return retval;
+		inode = &inode_buf;
+	}
+	memcpy((void *)inode->i_block, buf, EXT4_MIN_INLINE_DATA_SIZE);
+	retval = ext2fs_write_inode(fs, ino, inode);
+	if (retval)
+		return retval;
+	data.fs = fs;
+	data.ino = ino;
+	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
+	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
+	return ext2fs_inline_data_ea_set(&data);
+}
+
 #ifdef DEBUG
 #include "e2p/e2p.h"
 
diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
index ba8ed8e..a31912d 100644
--- a/lib/ext2fs/symlink.c
+++ b/lib/ext2fs/symlink.c
@@ -27,6 +27,7 @@
 
 #include "ext2_fs.h"
 #include "ext2fs.h"
+#include "ext2_ext_attr.h"
 
 errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 			 const char *name, char *target)
@@ -36,6 +37,8 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	ext2_ino_t		scratch_ino;
 	blk64_t			blk;
 	int			fastlink;
+	int                     inline_link = 0;
+	int                     max_inline;
 	unsigned int		target_len;
 	char			*block_buf = 0;
 
@@ -53,12 +56,27 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	 */
 	fastlink = (target_len < sizeof(inode.i_block));
 	if (!fastlink) {
-		retval = ext2fs_new_block2(fs, 0, 0, &blk);
-		if (retval)
-			goto cleanup;
-		retval = ext2fs_get_mem(fs->blocksize, &block_buf);
-		if (retval)
-			goto cleanup;
+		if (EXT2_HAS_INCOMPAT_FEATURE(fs->super,
+				EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
+			/* whether target string can be stored
+			in inline data area */
+			max_inline = EXT2_INODE_SIZE(fs->super) -
+					sizeof(struct ext2_inode_large)-
+					sizeof(__u32)-
+					sizeof(struct ext2_ext_attr_entry)-
+					strlen("data")-
+					sizeof(__u32)+
+					EXT4_MIN_INLINE_DATA_SIZE;
+			inline_link = (target_len < max_inline);
+		}
+		if (!inline_link) {
+			retval = ext2fs_new_block2(fs, 0, 0, &blk);
+			if (retval)
+				goto cleanup;
+			retval = ext2fs_get_mem(fs->blocksize, &block_buf);
+			if (retval)
+				goto cleanup;
+		}
 	}
 
 	/*
@@ -77,7 +95,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	memset(&inode, 0, sizeof(struct ext2_inode));
 	inode.i_mode = LINUX_S_IFLNK | 0777;
 	inode.i_uid = inode.i_gid = 0;
-	ext2fs_iblk_set(fs, &inode, fastlink ? 0 : 1);
+	ext2fs_iblk_set(fs, &inode, (fastlink || inline_link) ? 0 : 1);
 	inode.i_links_count = 1;
 	ext2fs_inode_size_set(fs, &inode, target_len);
 	/* The time fields are set by ext2fs_write_new_inode() */
@@ -85,6 +103,8 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	if (fastlink) {
 		/* Fast symlinks, target stored in inode */
 		strcpy((char *)&inode.i_block, target);
+	} else if (inline_link) {
+		inode.i_flags |= EXT4_INLINE_DATA_FL;
 	} else {
 		/* Slow symlinks, target stored in the first block */
 		memset(block_buf, 0, fs->blocksize);
@@ -109,14 +129,19 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 		goto cleanup;
 
 	if (!fastlink) {
-		retval = ext2fs_bmap2(fs, ino, &inode, NULL, BMAP_SET, 0, NULL,
-				      &blk);
-		if (retval)
-			goto cleanup;
+		if (inline_link) {
+			retval = ext2fs_symlink_inline_data_set(fs, ino, 0,
+						target, target_len);
+		} else {
+			retval = ext2fs_bmap2(fs, ino, &inode, NULL,
+						BMAP_SET, 0, NULL, &blk);
+		  if (retval)
+		    goto cleanup;
 
-		retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
-		if (retval)
-			goto cleanup;
+		  retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
+		  if (retval)
+		    goto cleanup;
+		}
 	}
 
 	/*
@@ -139,7 +164,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	/*
 	 * Update accounting....
 	 */
-	if (!fastlink)
+	if ((!fastlink) && (!inline_link))
 		ext2fs_block_alloc_stats2(fs, blk, +1);
 	ext2fs_inode_alloc_stats2(fs, ino, +1, 0);
 
-- 
1.9.1

Re: [PATCH] debugfs: Add inline data feature for symlink.

[Darrick J. Wong]

On Thu, Jul 31, 2014 at 02:58:52PM +0800, Pu Hou wrote:
> Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.

I think Ted prefers wrapping commit messages at 70 bytes.

> Signed-off-by: Pu Hou <houpu.hp@alibaba-inc.com>
> ---
>  lib/ext2fs/ext2fs.h      |  3 +++
>  lib/ext2fs/inline_data.c | 26 +++++++++++++++++++++++
>  lib/ext2fs/symlink.c     | 55 +++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 69 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 7812956..8268d96 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -1401,6 +1401,9 @@ extern errcode_t ext2fs_inline_data_get(ext2_filsys fs, ext2_ino_t ino,
>  extern errcode_t ext2fs_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
>  					struct ext2_inode *inode,
>  					void *buf, size_t size);
> +extern errcode_t ext2fs_symlink_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
> +					struct ext2_inode *inode,
> +					void *buf, size_t size);
>  
>  /* inode.c */
>  extern errcode_t ext2fs_create_inode_cache(ext2_filsys fs,
> diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
> index 19248a0..ed82a32 100644
> --- a/lib/ext2fs/inline_data.c
> +++ b/lib/ext2fs/inline_data.c
> @@ -585,6 +585,32 @@ errcode_t ext2fs_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
>  	return ext2fs_inline_data_ea_set(&data);
>  }
>  
> +errcode_t ext2fs_symlink_inline_data_set(ext2_filsys fs, ext2_ino_t ino,
> +				 struct ext2_inode *inode,
> +				 void *buf, size_t size)
> +{
> +	struct ext2_inode inode_buf;
> +	struct ext2_inline_data data;
> +	errcode_t retval;
> +	size_t free_ea_size, existing_size, free_inode_size;
> +
> +	if (!inode) {
> +		retval = ext2fs_read_inode(fs, ino, &inode_buf);
> +		if (retval)
> +			return retval;
> +		inode = &inode_buf;
> +	}
> +	memcpy((void *)inode->i_block, buf, EXT4_MIN_INLINE_DATA_SIZE);
> +	retval = ext2fs_write_inode(fs, ino, inode);
> +	if (retval)
> +		return retval;
> +	data.fs = fs;
> +	data.ino = ino;
> +	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
> +	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
> +	return ext2fs_inline_data_ea_set(&data);

Doesn't ext2fs_inline_data_set() suffice?

> +}
> +
>  #ifdef DEBUG
>  #include "e2p/e2p.h"
>  
> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> index ba8ed8e..a31912d 100644
> --- a/lib/ext2fs/symlink.c
> +++ b/lib/ext2fs/symlink.c
> @@ -27,6 +27,7 @@
>  
>  #include "ext2_fs.h"
>  #include "ext2fs.h"
> +#include "ext2_ext_attr.h"
>  
>  errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  			 const char *name, char *target)
> @@ -36,6 +37,8 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  	ext2_ino_t		scratch_ino;
>  	blk64_t			blk;
>  	int			fastlink;
> +	int                     inline_link = 0;
> +	int                     max_inline;
>  	unsigned int		target_len;
>  	char			*block_buf = 0;
>  
> @@ -53,12 +56,27 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  	 */
>  	fastlink = (target_len < sizeof(inode.i_block));
>  	if (!fastlink) {
> -		retval = ext2fs_new_block2(fs, 0, 0, &blk);
> -		if (retval)
> -			goto cleanup;
> -		retval = ext2fs_get_mem(fs->blocksize, &block_buf);
> -		if (retval)
> -			goto cleanup;
> +		if (EXT2_HAS_INCOMPAT_FEATURE(fs->super,
> +				EXT4_FEATURE_INCOMPAT_INLINE_DATA)) {
> +			/* whether target string can be stored
> +			in inline data area */
> +			max_inline = EXT2_INODE_SIZE(fs->super) -
> +					sizeof(struct ext2_inode_large)-
> +					sizeof(__u32)-
> +					sizeof(struct ext2_ext_attr_entry)-
> +					strlen("data")-
> +					sizeof(__u32)+
> +					EXT4_MIN_INLINE_DATA_SIZE;
> +			inline_link = (target_len < max_inline);

Does ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE return incorrect
results?  I don't think we should have all xattr specific stuff belongs here in
the symlink routines.

> +		}
> +		if (!inline_link) {
> +			retval = ext2fs_new_block2(fs, 0, 0, &blk);
> +			if (retval)
> +				goto cleanup;
> +			retval = ext2fs_get_mem(fs->blocksize, &block_buf);
> +			if (retval)
> +				goto cleanup;
> +		}
>  	}
>  
>  	/*
> @@ -77,7 +95,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  	memset(&inode, 0, sizeof(struct ext2_inode));
>  	inode.i_mode = LINUX_S_IFLNK | 0777;
>  	inode.i_uid = inode.i_gid = 0;
> -	ext2fs_iblk_set(fs, &inode, fastlink ? 0 : 1);
> +	ext2fs_iblk_set(fs, &inode, (fastlink || inline_link) ? 0 : 1);
>  	inode.i_links_count = 1;
>  	ext2fs_inode_size_set(fs, &inode, target_len);
>  	/* The time fields are set by ext2fs_write_new_inode() */
> @@ -85,6 +103,8 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  	if (fastlink) {
>  		/* Fast symlinks, target stored in inode */
>  		strcpy((char *)&inode.i_block, target);
> +	} else if (inline_link) {
> +		inode.i_flags |= EXT4_INLINE_DATA_FL;
>  	} else {
>  		/* Slow symlinks, target stored in the first block */
>  		memset(block_buf, 0, fs->blocksize);
> @@ -109,14 +129,19 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  		goto cleanup;
>  
>  	if (!fastlink) {
> -		retval = ext2fs_bmap2(fs, ino, &inode, NULL, BMAP_SET, 0, NULL,
> -				      &blk);
> -		if (retval)
> -			goto cleanup;
> +		if (inline_link) {
> +			retval = ext2fs_symlink_inline_data_set(fs, ino, 0,
> +						target, target_len);
> +		} else {
> +			retval = ext2fs_bmap2(fs, ino, &inode, NULL,
> +						BMAP_SET, 0, NULL, &blk);
> +		  if (retval)
> +		    goto cleanup;
>  
> -		retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
> -		if (retval)
> -			goto cleanup;
> +		  retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
> +		  if (retval)
> +		    goto cleanup;

Please fix the indentation inconsistency (tabs, not spaces).

> +		}
>  	}
>  
>  	/*
> @@ -139,7 +164,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
>  	/*
>  	 * Update accounting....
>  	 */
> -	if (!fastlink)
> +	if ((!fastlink) && (!inline_link))

Probably unnecessary to put !fastlink and !inline_link in parentheses.

--D
>  		ext2fs_block_alloc_stats2(fs, blk, +1);
>  	ext2fs_inode_alloc_stats2(fs, ino, +1, 0);
>  
> -- 
> 1.9.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: Add inline data feature for symlink.

[侯普(谷熠)]

On Thu, Jul 31, 2014 at 02:58:52PM +0800, Pu Hou wrote:
>> Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.
>
>I think Ted prefers wrapping commit messages at 70 bytes.
>
Thanks for reminding me. I will fix it.

>> +	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
>> +	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
>> +	return ext2fs_inline_data_ea_set(&data);
>
>Doesn't ext2fs_inline_data_set() suffice?
>
It is unnecessary to use this dedicated function.

>> +					sizeof(__u32)+
>> +					EXT4_MIN_INLINE_DATA_SIZE;
>> +			inline_link = (target_len < max_inline);
>
>Does ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE return incorrect
>results?  I don't think we should have all xattr specific stuff belongs here in
>the symlink routines.

Sorry. I did not notice that some xattr might be existed  when an inode
number is passed to ext2fs_symlink(). But, as far as I can see, when 0
is pass as inode number. ext2fs_xattr_inode_max_size() will read inode
by ext2fs_read_inode_full(). But at that time, we have not writen the
inode back. and the size of inline data for symlink is uncertain.
I will try to find a workaround.

>> -		if (retval)
>> -			goto cleanup;
>> +		  retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
>> +		  if (retval)
>> +		    goto cleanup;
>
>Please fix the indentation inconsistency (tabs, not spaces).

>> -	if (!fastlink)
>> +	if ((!fastlink) && (!inline_link))
>
>Probably unnecessary to put !fastlink and !inline_link in parentheses.
>
>--D
Darrick, thank again for your generous help.

Re: [PATCH] debugfs: Add inline data feature for symlink.

[Darrick J. Wong]

On Fri, Aug 01, 2014 at 06:37:56PM +0800, 侯普(谷熠) wrote:
> On Thu, Jul 31, 2014 at 02:58:52PM +0800, Pu Hou wrote:
> >> Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.
> >
> >I think Ted prefers wrapping commit messages at 70 bytes.
> >
> Thanks for reminding me. I will fix it.
> 
> >> +	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
> >> +	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
> >> +	return ext2fs_inline_data_ea_set(&data);
> >
> >Doesn't ext2fs_inline_data_set() suffice?
> >
> It is unnecessary to use this dedicated function.

Why do you say it's unnecessary?  The dedicated function does (or at least is
supposed to) do exactly what you want.

Now I'm wondering -- why worry about size at all?  You can modify
ext2fs_symlink to call ext2fs_inline_data_set() directly.  If there's not
enough room, it'll return EXT2_ET_INLINE_DATA_NO_SPACE and you can try again
with the classic stuff-it-in-a-block method.

(Of course, if you try this and the API turns not to behave as advertised I'd
like to hear about that too. :))

--D

> 
> >> +					sizeof(__u32)+
> >> +					EXT4_MIN_INLINE_DATA_SIZE;
> >> +			inline_link = (target_len < max_inline);
> >
> >Does ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE return incorrect
> >results?  I don't think we should have all xattr specific stuff belongs here in
> >the symlink routines.
> 
> Sorry. I did not notice that some xattr might be existed  when an inode
> number is passed to ext2fs_symlink(). But, as far as I can see, when 0
> is pass as inode number. ext2fs_xattr_inode_max_size() will read inode
> by ext2fs_read_inode_full(). But at that time, we have not writen the
> inode back. and the size of inline data for symlink is uncertain.
> I will try to find a workaround.
> 
> >> -		if (retval)
> >> -			goto cleanup;
> >> +		  retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
> >> +		  if (retval)
> >> +		    goto cleanup;
> >
> >Please fix the indentation inconsistency (tabs, not spaces).
> 
> >> -	if (!fastlink)
> >> +	if ((!fastlink) && (!inline_link))
> >
> >Probably unnecessary to put !fastlink and !inline_link in parentheses.
> >
> >--D
> Darrick, thank again for your generous help.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: Add inline data feature for symlink.

[Pu Hou]

On Mon, Aug 04, 2014 at 02:53:41PM +0800, Pu Hou wrote:
> On Sat, Aug 02, 2014 at 12:48:10AM +0800, Darrick J. Wong wrote:
> > On Fri, Aug 01, 2014 at 06:37:56PM +0800, Pu Hou wrote:
> > > On Thu, Jul 31, 2014 at 02:58:52PM +0800, Pu Hou wrote:
> > > >> Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.
> > > >
> > > >I think Ted prefers wrapping commit messages at 70 bytes.
> > > >
> > > Thanks for reminding me. I will fix it.
> > > 
> > > >> +	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
> > > >> +	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
> > > >> +	return ext2fs_inline_data_ea_set(&data);
> > > >
> > > >Doesn't ext2fs_inline_data_set() suffice?
> > > >
> > > It is unnecessary to use this dedicated function.
> > 
> > Why do you say it's unnecessary?  The dedicated function does (or at least is
> > supposed to) do exactly what you want.
> > 
> 
> Compare with the dedicated function, ext2fs_inline_data_set() will check
> the available xattr size and inline data size by read the inode and do
> some calculation. And I thought some "dirty data" of an inode on disk might
> have an effect on the calculated available size.
> 
> Now I find that this concern is unnecessary. Because ext2fs_write_new_inode()
> which is called before will make sure the xattr of the inode are clean.
> So there is no need to use this dedicated function.
> 
> > Now I'm wondering -- why worry about size at all?  You can modify
> > ext2fs_symlink to call ext2fs_inline_data_set() directly.  If there's not
> > enough room, it'll return EXT2_ET_INLINE_DATA_NO_SPACE and you can try again
> > with the classic stuff-it-in-a-block method.
> > 
> > (Of course, if you try this and the API turns not to behave as advertised I'd
> > like to hear about that too. :))
> 
> That sounds nice. I will try in this way and give a feedback later.
>
I tried to call ext2fs_inline_data_set() from ext2fs_symlink. The result is
not as expected. Before it gets to "return EXT2_ET_INLINE_DATA_NO_SPACE",
ext2fs_inline_data_size() is called. In this function, it may return at here:

	if (!(inode.i_flags & EXT4_INLINE_DATA_FL))
		return EXT2_ET_NO_INLINE_DATA;

and the message was printed as below:

ext2fs_symlink: Inode doesn't have inline data
symlink: Inode doesn't have inline data

That is because we did not mark EXT4_INLINE_DATA_FL yet. Even if mark the flag,
it still return at this point:

	data.fs = fs;
	data.ino = ino;
	retval = ext2fs_inline_data_ea_get(&data);
	if (retval)
		return retval;

and the message was printed as below:

ext2fs_symlink: Extended attribute key not found
symlink: Extended attribute key not found

That means the xattr of the new created inode is clean.
because ext2fs_write_new_inode() is called early, the xattr is made clean.

I think use "ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE"
to get the available xattr size. And use ext2fs_inline_data_ea_set()
directly to set inline data is a workaround.

 
> > 
> > --D
> > 
> > > 
> > > >> +					sizeof(__u32)+
> > > >> +					EXT4_MIN_INLINE_DATA_SIZE;
> > > >> +			inline_link = (target_len < max_inline);
> > > >
> > > >Does ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE return incorrect
> > > >results?  I don't think we should have all xattr specific stuff belongs here in
> > > >the symlink routines.
> > > 
> > > Sorry. I did not notice that some xattr might be existed  when an inode
> > > number is passed to ext2fs_symlink(). But, as far as I can see, when 0
> > > is pass as inode number. ext2fs_xattr_inode_max_size() will read inode
> > > by ext2fs_read_inode_full(). But at that time, we have not writen the
> > > inode back. and the size of inline data for symlink is uncertain.
> > > I will try to find a workaround.
> > > 
> > > >> -		if (retval)
> > > >> -			goto cleanup;
> > > >> +		  retval = io_channel_write_blk64(fs->io, blk, 1, block_buf);
> > > >> +		  if (retval)
> > > >> +		    goto cleanup;
> > > >
> > > >Please fix the indentation inconsistency (tabs, not spaces).
> > > 
> > > >> -	if (!fastlink)
> > > >> +	if ((!fastlink) && (!inline_link))
> > > >
> > > >Probably unnecessary to put !fastlink and !inline_link in parentheses.
> > > >
> > > >--D
> > > Darrick, thank again for your generous help.
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: Add inline data feature for symlink.

[Darrick J. Wong]

On Tue, Aug 05, 2014 at 08:27:58PM +0800, Pu Hou wrote:
> On Mon, Aug 04, 2014 at 02:53:41PM +0800, Pu Hou wrote:
> > On Sat, Aug 02, 2014 at 12:48:10AM +0800, Darrick J. Wong wrote:
> > > On Fri, Aug 01, 2014 at 06:37:56PM +0800, Pu Hou wrote:
> > > > On Thu, Jul 31, 2014 at 02:58:52PM +0800, Pu Hou wrote:
> > > > >> Symlink in debugfs can take advantage of inline data feature. The path name of the target of the symbol link which is longer than 60 byte and shorter than 132 byte can stay in inline data area.
> > > > >
> > > > >I think Ted prefers wrapping commit messages at 70 bytes.
> > > > >
> > > > Thanks for reminding me. I will fix it.
> > > > 
> > > > >> +	data.ea_size = size - EXT4_MIN_INLINE_DATA_SIZE;
> > > > >> +	data.ea_data = buf + EXT4_MIN_INLINE_DATA_SIZE;
> > > > >> +	return ext2fs_inline_data_ea_set(&data);
> > > > >
> > > > >Doesn't ext2fs_inline_data_set() suffice?
> > > > >
> > > > It is unnecessary to use this dedicated function.
> > > 
> > > Why do you say it's unnecessary?  The dedicated function does (or at least is
> > > supposed to) do exactly what you want.
> > > 
> > 
> > Compare with the dedicated function, ext2fs_inline_data_set() will check
> > the available xattr size and inline data size by read the inode and do
> > some calculation. And I thought some "dirty data" of an inode on disk might
> > have an effect on the calculated available size.
> > 
> > Now I find that this concern is unnecessary. Because ext2fs_write_new_inode()
> > which is called before will make sure the xattr of the inode are clean.
> > So there is no need to use this dedicated function.
> > 
> > > Now I'm wondering -- why worry about size at all?  You can modify
> > > ext2fs_symlink to call ext2fs_inline_data_set() directly.  If there's not
> > > enough room, it'll return EXT2_ET_INLINE_DATA_NO_SPACE and you can try again
> > > with the classic stuff-it-in-a-block method.
> > > 
> > > (Of course, if you try this and the API turns not to behave as advertised I'd
> > > like to hear about that too. :))
> > 
> > That sounds nice. I will try in this way and give a feedback later.
> >
> I tried to call ext2fs_inline_data_set() from ext2fs_symlink. The result is
> not as expected. Before it gets to "return EXT2_ET_INLINE_DATA_NO_SPACE",
> ext2fs_inline_data_size() is called. In this function, it may return at here:
> 
> 	if (!(inode.i_flags & EXT4_INLINE_DATA_FL))
> 		return EXT2_ET_NO_INLINE_DATA;
> 
> and the message was printed as below:
> 
> ext2fs_symlink: Inode doesn't have inline data
> symlink: Inode doesn't have inline data

Yeah, you have to set EXT4_INLINE_DATA_FL then write_new_inode before calling
ext2fs_inline_data_set() so that the subsequent inlinedata calls (which re-read
the inode off the disk) see the flag.  (Yuck)

> That is because we did not mark EXT4_INLINE_DATA_FL yet. Even if mark the flag,
> it still return at this point:
> 
> 	data.fs = fs;
> 	data.ino = ino;
> 	retval = ext2fs_inline_data_ea_get(&data);
> 	if (retval)
> 		return retval;
> 
> and the message was printed as below:
> 
> ext2fs_symlink: Extended attribute key not found
> symlink: Extended attribute key not found

Personally, I think that's a bug in inline_data.c.  Retrieving any of the
inline data shouldn't fail just because there's no EA -- what if the EA is
damaged?  For the case of creating a symlink, the EA doesn't exist because we
haven't finished creating the file yet.

I think it makes more sense for ext2fs_inline_data_ea_get() to check for
EXT2_ET_EA_KEY_NOT_FOUND when it tries to grab the EA, and simply set
data.ea_size = 0.

> That means the xattr of the new created inode is clean.
> because ext2fs_write_new_inode() is called early, the xattr is made clean.

If you've already called write_new_inode (for the inline symlink case) then you
must call write_inode or else you see the behavior you observed.

> I think use "ext2fs_xattr_inode_max_size() + EXT4_MIN_INLINE_DATA_SIZE"
> to get the available xattr size. And use ext2fs_inline_data_ea_set()
> directly to set inline data is a workaround.

There should provide a test case.  Ted seems to be getting stricter about that.

Oh well, how about this patch?  Seeing as I'm messing around with the inline
data patchbomb some more anyway.

--D

diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
index 2e1fc68..68f8893 100644
--- a/lib/ext2fs/symlink.c
+++ b/lib/ext2fs/symlink.c
@@ -35,7 +35,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	struct ext2_inode	inode;
 	ext2_ino_t		scratch_ino;
 	blk64_t			blk;
-	int			fastlink;
+	int			fastlink, inlinelink;
 	unsigned int		target_len;
 	char			*block_buf = 0;
 
@@ -78,15 +78,36 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	memset(&inode, 0, sizeof(struct ext2_inode));
 	inode.i_mode = LINUX_S_IFLNK | 0777;
 	inode.i_uid = inode.i_gid = 0;
-	ext2fs_iblk_set(fs, &inode, fastlink ? 0 : 1);
 	inode.i_links_count = 1;
 	ext2fs_inode_size_set(fs, &inode, target_len);
 	/* The time fields are set by ext2fs_write_new_inode() */
 
+	inlinelink = !fastlink &&
+		     (fs->super->s_feature_incompat &
+					EXT4_FEATURE_INCOMPAT_INLINE_DATA) &&
+		     (target_len < fs->blocksize);
 	if (fastlink) {
 		/* Fast symlinks, target stored in inode */
 		strcpy((char *)&inode.i_block, target);
+	} else if (inlinelink) {
+		/* Try inserting an inline data symlink */
+		inode.i_flags |= EXT4_INLINE_DATA_FL;
+		retval = ext2fs_write_new_inode(fs, ino, &inode);
+		if (retval)
+			goto cleanup;
+		retval = ext2fs_inline_data_set(fs, ino, &inode, target,
+						target_len);
+		if (retval) {
+			inode.i_flags &= ~EXT4_INLINE_DATA_FL;
+			inlinelink = 0;
+			goto need_block;
+		}
+		retval = ext2fs_read_inode(fs, ino, &inode);
+		if (retval)
+			goto cleanup;
 	} else {
+need_block:
+		ext2fs_iblk_set(fs, &inode, 1);
 		/* Slow symlinks, target stored in the first block */
 		memset(block_buf, 0, fs->blocksize);
 		strcpy(block_buf, target);
@@ -105,11 +126,14 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	 * number is assigned by write_new_inode, which means that the
 	 * operations using ino must come after it.
 	 */
-	retval = ext2fs_write_new_inode(fs, ino, &inode);
+	if (inlinelink)
+		retval = ext2fs_write_inode(fs, ino, &inode);
+	else
+		retval = ext2fs_write_new_inode(fs, ino, &inode);
 	if (retval)
 		goto cleanup;
 
-	if (!fastlink) {
+	if (!fastlink && !inlinelink) {
 		retval = ext2fs_bmap2(fs, ino, &inode, NULL, BMAP_SET, 0, NULL,
 				      &blk);
 		if (retval)
@@ -140,7 +164,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
 	/*
 	 * Update accounting....
 	 */
-	if (!fastlink)
+	if (!fastlink && !inlinelink)
 		ext2fs_block_alloc_stats2(fs, blk, +1);
 	ext2fs_inode_alloc_stats2(fs, ino, +1, 0);