Re: [PATCH] Large EAs in ext4

[Mingming Cao <cmm@us.ibm.com>]

在 2008-08-25一的 16:47 -0600，Andreas Dilger写道：
> On Aug 25, 2008  21:30 +0530, Kalpak Shah wrote:
> > This is the implementation for large EA support in ext4. Note that this
> > also helps to have a larger number of EAs since large EAs get written
> > out to a new inode instead of the EA block.
> > 
> > If value of an attribute is greater than 2048 bytes the value is not
> > saved in the external EA block, instead it is saved in an inode.
> 
> I just realized that this needs to be (blocksize / 2) instead of 2048,
> or we will never get the EA inodes in case of 1kB/2kB block filesystem
> where we need it the most.
> 
> > +struct  inode *ext4_xattr_inode_iget(struct inode *parent, int ea_ino, int *err)
>          ^^ extra space here
> 
> > +	if (ea_inode->i_mtime.tv_sec != parent->i_ino ||
> 
> Do you think it makes sense to "#define i_xattr_inode_parent i_mtime.tv_sec"
> in case there is a decision to change which field is used?  Or do people
> think that is more confusing than helpful?
> 
> Note to readers that this is a new patch, and Lustre doesn't use it yet,
> but we'd like to in the relatively near future so feedback that affects
> the on disk format is preferred sooner than later.
> 
> > +ext4_xattr_inode_set(handle_t *handle, struct inode *inode, int *ea_ino,
> > +		     const void *value, size_t value_len)
> > +{
> > +	/*
> > +	 * Make sure that enough buffer credits are available else extend the
> > +	 * transaction.
> > +	 */
> > +	req_buffer_credits = (value_len / inode->i_sb->s_blocksize) + 4;
> 
> Can you please explain in the comment what the "+ 4" blocks are?
> I suspect this will not be enough if the xattr is large, it should just
> use one of the standard "transaction size" helper functions to determine
> metadata size.
> 

ext4_meta_trans_blocks() will gives the metadata size, which also
account for block group bitmap and block group descriptor blocks,
superblock, inode block.

also here

> @@ -1059,10 +1352,23 @@ ext4_xattr_set(struct inode *inode, int 
>                const void *value, size_t value_len, int flags)
>  {
>         handle_t *handle;
> +       int buffer_credits;
>         int error, retries = 0;

> +       buffer_credits = EXT4_DATA_TRANS_BLOCKS(inode->i_sb);
> +       if ((value_len >= EXT4_XATTR_MIN_LARGE_EA_SIZE) &&
> +           (EXT4_SB(inode->i_sb)->s_es->s_feature_incompat &
> +            EXT4_FEATURE_INCOMPAT_EA_INODE)) {
> +               /* For new inode */
> +               buffer_credits +=  > > >EXT4_SINGLEDATA_TRANS_BLOCKS(inode->i_sb) +3;
> +
> +               /* For the blocks to be written in the EA inode */
> +               buffer_credits += (value_len + inode->i_sb->s_blocksize - 1) /
> +                                  inode->i_sb->s_blocksize;
> +       }
> +
 > retry:
> -       handle = ext4_journal_start(inode, > EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
> +       handle = ext4_journal_start(inode, buffer_credits);
>         if (IS_ERR(handle)) {
>                 error = PTR_ERR(handle);
>         } else {


the credits calculation could be replaced with something like this:

nrblocks = (value_len + inode->i_sb->s_blocksize - 1) / inode->i_sb->s_blocksize;
buffer_credits =  ext4_meta_trans_blocks(inode,  nrblocks,  1) + nrblocks;



BTW, I  think we should update the xttars credits micro in ext4_jbd2.h,
that is based on 1 xattr block assumption...

/* Extended attribute operations touch at most two data buffers,
 * two bitmap buffers, and two group summaries, in addition to the inode
 * and the superblock, which are already accounted for. */

#define EXT4_XATTR_TRANS_BLOCKS         6U



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