lkml.org 
[lkml]   [2011]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V1 02/17] ext4: Add the basic function for inline data support.
From
Date
On 2011-10-26, at 1:34 AM, Tao Ma wrote:
> Implement inline data with xattr. This idea is inspired by Andreas.
> So now we use "system.data" to store xattr. For inode_size = 256,
> currently we uses all the space between i_extra_isize and inode_size.
> For inode_size > 256, we use half of that space.
>
> +#define EXT4_XATTR_SYSTEM_DATA_NAME "data"

Did you check whether the "data" string takes 4 bytes or 8 in the
xattr header? I believe it is storing the NUL termination (and
the code is using "sizeof(EXT4_XATTR_SYSTEM_DATA_NAME)" = 5), so
I believe it will round up to the next 4-byte boundary. Using a
3-byte name will save a bit of space, like "system.dat".

> +void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
> + void *buffer, loff_t pos, unsigned len)
> +{
> + header = IHDR(inode, ext4_raw_inode(iloc));
> + entry = (struct ext4_xattr_entry *)((void *)ext4_raw_inode(iloc) +
> + EXT4_I(inode)->i_inline_off);
> + memcpy((void *)IFIRST(header) + le16_to_cpu(entry->e_value_offs) + pos,
> + buffer + pos, len);
> +}
> +
> +int ext4_init_inline_data(handle_t *handle, struct inode *inode,
> + struct ext4_iloc *iloc)
> +{
> + size = ext4_get_max_inline_size(inode);
> + value = kzalloc(size, GFP_NOFS);
> + if (!value)
> + return -ENOMEM;
> +
> + error = ext4_xattr_ibody_set(handle, inode, &i, &is);
> +}

Since file data is changed very rarely, instead of consuming the full
xattr space that may not be needed, wouldn't it be better to change
ext4_write_inline_data() to just to the ext4_xattr_ibody_set() to save
the exact-sized buffer into the xattr? That will allow other xattrs
to be stored in this space as well as the inline data.


Cheers, Andreas







\
 
 \ /
  Last update: 2011-10-26 10:39    [W:0.100 / U:1.848 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site