diff mbox

[10/49] libext2fs: fix iblocks correctly when expanding an inline_data file

Message ID 20140311065502.30585.81788.stgit@birch.djwong.org
State Accepted, archived
Headers show

Commit Message

Darrick Wong March 11, 2014, 6:55 a.m. UTC 
i_blocks covers the number of blocks allocated to an inode for data,
extents, and ACL blocks.  Since it's possible for a file to have a
separate ACL block and inline data, we must be careful when expanding
an inline data file to adjust, not set, the value of i_blocks.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/inline_data.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)



--
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

Comments

Andreas Dilger March 12, 2014, 4:38 p.m. UTC | #1 
I thought it wasn't possible to have inline data and an external xattr block?  It doesn't make sense to do it that way compared to the normal inline xattr and external data block. 

Cheers, Andreas

> On Mar 11, 2014, at 0:55, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> 
> i_blocks covers the number of blocks allocated to an inode for data,
> extents, and ACL blocks.  Since it's possible for a file to have a
> separate ACL block and inline data, we must be careful when expanding
> an inline data file to adjust, not set, the value of i_blocks.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> lib/ext2fs/inline_data.c |    5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
> index 72e8fa3..a9ec923 100644
> --- a/lib/ext2fs/inline_data.c
> +++ b/lib/ext2fs/inline_data.c
> @@ -372,7 +372,9 @@ ext2fs_inline_data_dir_expand(ext2_filsys fs, ext2_ino_t ino,
>    if (EXT2_HAS_INCOMPAT_FEATURE(fs->super, EXT3_FEATURE_INCOMPAT_EXTENTS))
>        inode->i_flags |= EXT4_EXTENTS_FL;
>    inode->i_flags &= ~EXT4_INLINE_DATA_FL;
> -    ext2fs_iblk_set(fs, inode, 1);
> +    retval = ext2fs_iblk_add_blocks(fs, inode, 1);
> +    if (retval)
> +        goto errout;
>    inode->i_size = fs->blocksize;
>    retval = ext2fs_bmap2(fs, ino, inode, 0, BMAP_SET, 0, 0, &blk);
>    if (retval)
> @@ -410,7 +412,6 @@ ext2fs_inline_data_file_expand(ext2_filsys fs, ext2_ino_t ino,
>        inode->i_flags |= EXT4_EXTENTS_FL;
>    }
>    inode->i_flags &= ~EXT4_INLINE_DATA_FL;
> -    ext2fs_iblk_set(fs, inode, 0);
>    inode->i_size = 0;
>    retval = ext2fs_write_inode(fs, ino, inode);
>    if (retval)
> 
> --
> 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
Darrick Wong March 12, 2014, 5:01 p.m. UTC | #2 
On Wed, Mar 12, 2014 at 10:38:57AM -0600, Andreas Dilger wrote:
> I thought it wasn't possible to have inline data and an external xattr block?
> It doesn't make sense to do it that way compared to the normal inline xattr
> and external data block. 

I don't know if you're /supposed/ to be able to do that, but the kernel permits
me to create an inline data dir and then set a huge xattr on it that forces the
allocation of an EA block.

--D
> 
> Cheers, Andreas
> 
> > On Mar 11, 2014, at 0:55, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> > 
> > i_blocks covers the number of blocks allocated to an inode for data,
> > extents, and ACL blocks.  Since it's possible for a file to have a
> > separate ACL block and inline data, we must be careful when expanding
> > an inline data file to adjust, not set, the value of i_blocks.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > lib/ext2fs/inline_data.c |    5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
> > index 72e8fa3..a9ec923 100644
> > --- a/lib/ext2fs/inline_data.c
> > +++ b/lib/ext2fs/inline_data.c
> > @@ -372,7 +372,9 @@ ext2fs_inline_data_dir_expand(ext2_filsys fs, ext2_ino_t ino,
> >    if (EXT2_HAS_INCOMPAT_FEATURE(fs->super, EXT3_FEATURE_INCOMPAT_EXTENTS))
> >        inode->i_flags |= EXT4_EXTENTS_FL;
> >    inode->i_flags &= ~EXT4_INLINE_DATA_FL;
> > -    ext2fs_iblk_set(fs, inode, 1);
> > +    retval = ext2fs_iblk_add_blocks(fs, inode, 1);
> > +    if (retval)
> > +        goto errout;
> >    inode->i_size = fs->blocksize;
> >    retval = ext2fs_bmap2(fs, ino, inode, 0, BMAP_SET, 0, 0, &blk);
> >    if (retval)
> > @@ -410,7 +412,6 @@ ext2fs_inline_data_file_expand(ext2_filsys fs, ext2_ino_t ino,
> >        inode->i_flags |= EXT4_EXTENTS_FL;
> >    }
> >    inode->i_flags &= ~EXT4_INLINE_DATA_FL;
> > -    ext2fs_iblk_set(fs, inode, 0);
> >    inode->i_size = 0;
> >    retval = ext2fs_write_inode(fs, ino, inode);
> >    if (retval)
> > 
> > --
> > 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
--
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
Theodore Ts'o March 14, 2014, 1:25 p.m. UTC | #3 
On Wed, Mar 12, 2014 at 10:01:33AM -0700, Darrick J. Wong wrote:
> On Wed, Mar 12, 2014 at 10:38:57AM -0600, Andreas Dilger wrote:
> > I thought it wasn't possible to have inline data and an external xattr block?
> > It doesn't make sense to do it that way compared to the normal inline xattr
> > and external data block. 
> 
> I don't know if you're /supposed/ to be able to do that, but the kernel permits
> me to create an inline data dir and then set a huge xattr on it that forces the
> allocation of an EA block.

I could imagine situations where the file size is very small, and the
xattr size is close to the blocksize, where it would make sense to
allow this (since if we prohibited this, we would need to use two
blocks, one for the data block since the data does't fit in the inode,
and a second data block for the xattr)

Anyway, I've applid the patch, thanks.

					- Ted
--
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
diff mbox

Patch

diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
index 72e8fa3..a9ec923 100644
--- a/lib/ext2fs/inline_data.c
+++ b/lib/ext2fs/inline_data.c
@@ -372,7 +372,9 @@  ext2fs_inline_data_dir_expand(ext2_filsys fs, ext2_ino_t ino,
 	if (EXT2_HAS_INCOMPAT_FEATURE(fs->super, EXT3_FEATURE_INCOMPAT_EXTENTS))
 		inode->i_flags |= EXT4_EXTENTS_FL;
 	inode->i_flags &= ~EXT4_INLINE_DATA_FL;
-	ext2fs_iblk_set(fs, inode, 1);
+	retval = ext2fs_iblk_add_blocks(fs, inode, 1);
+	if (retval)
+		goto errout;
 	inode->i_size = fs->blocksize;
 	retval = ext2fs_bmap2(fs, ino, inode, 0, BMAP_SET, 0, 0, &blk);
 	if (retval)
@@ -410,7 +412,6 @@  ext2fs_inline_data_file_expand(ext2_filsys fs, ext2_ino_t ino,
 		inode->i_flags |= EXT4_EXTENTS_FL;
 	}
 	inode->i_flags &= ~EXT4_INLINE_DATA_FL;
-	ext2fs_iblk_set(fs, inode, 0);
 	inode->i_size = 0;
 	retval = ext2fs_write_inode(fs, ino, inode);
 	if (retval)