From: Zheng Liu <gnehzuil.liu@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-ext4@vger.kernel.org, Zheng Liu <wenqing.lz@taobao.com>,
Theodore Ts'o <tytso@mit.edu>
Subject: Re: [PATCH v1 03/22] libext2fs: add functions to operate on extended attribute
Date: Sat, 12 Oct 2013 16:41:11 +0800 [thread overview]
Message-ID: <20131012084111.GF6462@gmail.com> (raw)
In-Reply-To: <20131012083200.GD14971@birch.djwong.org>
On Sat, Oct 12, 2013 at 01:32:00AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 12, 2013 at 01:51:54PM +0800, Zheng Liu wrote:
> > On Fri, Oct 11, 2013 at 03:51:15PM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 02, 2013 at 05:49:30PM +0800, Zheng Liu wrote:
> > > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > >
> > > > We need to define some functions to operate extended attribute in order
> > > > to support inline data.
> > > >
> > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> > > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > > ---
> > > > lib/ext2fs/ext2_err.et.in | 3 +
> > > > lib/ext2fs/ext2_ext_attr.h | 31 ++++++++
> > > > lib/ext2fs/ext2fs.h | 9 +++
> > > > lib/ext2fs/ext_attr.c | 186 ++++++++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 229 insertions(+)
> > > >
> > > > diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> > > > index d20c6b7..7e6d6e5 100644
> > > > --- a/lib/ext2fs/ext2_err.et.in
> > > > +++ b/lib/ext2fs/ext2_err.et.in
> > > > @@ -476,4 +476,7 @@ ec EXT2_ET_MMP_CSUM_INVALID,
> > > > ec EXT2_ET_FILE_EXISTS,
> > > > "Ext2 file already exists"
> > > >
> > > > +ec EXT2_ET_EXT_ATTR_CURRUPTED,
> > > > + "Extended attribute currupted"
> > > > +
> > > > end
> > > > diff --git a/lib/ext2fs/ext2_ext_attr.h b/lib/ext2fs/ext2_ext_attr.h
> > > > index bbb0aaa..99ad849 100644
> > > > --- a/lib/ext2fs/ext2_ext_attr.h
> > > > +++ b/lib/ext2fs/ext2_ext_attr.h
> > > > @@ -15,6 +15,10 @@
> > > > /* Maximum number of references to one attribute block */
> > > > #define EXT2_EXT_ATTR_REFCOUNT_MAX 1024
> > > >
> > > > +/* Name indexes */
> > > > +#define EXT4_EXT_ATTR_INDEX_SYSTEM 7
> > > > +#define EXT4_EXT_ATTR_SYSTEM_DATA "data"
> > > > +
> > > > struct ext2_ext_attr_header {
> > > > __u32 h_magic; /* magic number for identification */
> > > > __u32 h_refcount; /* reference count */
> > > > @@ -25,6 +29,10 @@ struct ext2_ext_attr_header {
> > > > __u32 h_reserved[3]; /* zero right now */
> > > > };
> > > >
> > > > +struct ext2_ext_attr_ibody_header {
> > > > + __u32 h_magic;
> > > > +};
> > > > +
> > > > struct ext2_ext_attr_entry {
> > > > __u8 e_name_len; /* length of name */
> > > > __u8 e_name_index; /* attribute name index */
> > > > @@ -57,6 +65,29 @@ struct ext2_ext_attr_entry {
> > > > #define EXT2_XATTR_SIZE(size) \
> > > > (((size) + EXT2_EXT_ATTR_ROUND) & ~EXT2_EXT_ATTR_ROUND)
> > > >
> > > > +#define IHDR(inode) \
> > > > + ((struct ext2_ext_attr_ibody_header *) \
> > > > + ((char *)inode + \
> > > > + EXT2_GOOD_OLD_INODE_SIZE + \
> > > > + inode->i_extra_isize))
> > > > +#define IFIRST(hdr) ((struct ext2_ext_attr_entry *)((hdr)+1))
> > > > +#define EXT2_ZERO_EXT_ATTR_VALUE ((void *)-1)
> > > > +
> > > > +struct ext2_ext_attr_info {
> > > > + int name_index;
> > > > + const char *name;
> > > > + const void *value;
> > > > + size_t value_len;
> > > > +};
> > > > +
> > > > +struct ext2_ext_attr_search {
> > > > + struct ext2_ext_attr_entry *first;
> > > > + void *base;
> > > > + void *end;
> > > > + struct ext2_ext_attr_entry *here;
> > > > + int not_found;
> > > > +};
> > > > +
> > > > #ifdef __KERNEL__
> > > > # ifdef CONFIG_EXT2_FS_EXT_ATTR
> > > > extern int ext2_get_ext_attr(struct inode *, const char *, char *, size_t, int);
> > > > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> > > > index 3346c00..8c30197 100644
> > > > --- a/lib/ext2fs/ext2fs.h
> > > > +++ b/lib/ext2fs/ext2fs.h
> > > > @@ -1136,6 +1136,15 @@ extern errcode_t ext2fs_adjust_ea_refcount3(ext2_filsys fs, blk64_t blk,
> > > > char *block_buf,
> > > > int adjust, __u32 *newcount,
> > > > ext2_ino_t inum);
> > > > +extern errcode_t ext2fs_ext_attr_ibody_find(ext2_filsys fs,
> > > > + struct ext2_inode_large *inode,
> > > > + struct ext2_ext_attr_info *i,
> > > > + struct ext2_ext_attr_search *s);
> > > > +extern errcode_t ext2fs_ext_attr_find_entry(struct ext2_ext_attr_entry **pentry,
> > > > + int name_index, const char *name,
> > > > + size_t size, int sorted);
> > > > +extern errcode_t ext2fs_ext_attr_set_entry(struct ext2_ext_attr_info *i,
> > > > + struct ext2_ext_attr_search *s);
> > > >
> > > > /* extent.c */
> > > > extern errcode_t ext2fs_extent_header_verify(void *ptr, int size);
> > > > diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
> > > > index 9649a14..6d55340 100644
> > > > --- a/lib/ext2fs/ext_attr.c
> > > > +++ b/lib/ext2fs/ext_attr.c
> > > > @@ -186,3 +186,189 @@ errcode_t ext2fs_adjust_ea_refcount(ext2_filsys fs, blk_t blk,
> > > > return ext2fs_adjust_ea_refcount2(fs, blk, block_buf, adjust,
> > > > newcount);
> > > > }
> > > > +
> > > > +static errcode_t
> > > > +ext2fs_ext_attr_check_names(struct ext2_ext_attr_entry *entry, void *end)
> > > > +{
> > > > + while (!EXT2_EXT_IS_LAST_ENTRY(entry)) {
> > > > + struct ext2_ext_attr_entry *next = EXT2_EXT_ATTR_NEXT(entry);
> > > > + if ((void *)next >= end)
> > > > + return EXT2_ET_EXT_ATTR_CURRUPTED;
> > > > + entry = next;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static inline errcode_t
> > > > +ext2fs_ext_attr_check_entry(struct ext2_ext_attr_entry *entry, size_t size)
> > > > +{
> > > > + size_t value_size = entry->e_value_size;
> > > > +
> > > > + if (entry->e_value_block != 0 || value_size > size ||
> > > > + entry->e_value_offs + value_size > size)
> > > > + return EXT2_ET_EXT_ATTR_CURRUPTED;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +errcode_t ext2fs_ext_attr_find_entry(struct ext2_ext_attr_entry **pentry,
> > > > + int name_index, const char *name,
> > > > + size_t size, int sorted)
> > > > +{
> > > > + struct ext2_ext_attr_entry *entry;
> > > > + size_t name_len;
> > > > + int cmp = 1;
> > > > +
> > > > + if (name == NULL)
> > > > + return EXT2_ET_INVALID_ARGUMENT;
> > > > + name_len = strlen(name);
> > > > + for (entry = *pentry; !EXT2_EXT_IS_LAST_ENTRY(entry);
> > > > + entry = EXT2_EXT_ATTR_NEXT(entry)) {
> > > > + cmp = name_index - entry->e_name_index;
> > > > + if (!cmp)
> > > > + cmp = name_len - entry->e_name_len;
> > > > + if (!cmp)
> > > > + cmp = memcmp(name, EXT2_EXT_ATTR_NAME(entry),
> > > > + name_len);
> > > > + if (cmp <= 0 && (sorted || cmp == 0))
> > > > + break;
> > > > + }
> > > > + *pentry = entry;
> > > > + if (!cmp && ext2fs_ext_attr_check_entry(entry, size))
> > > > + return EXT2_ET_EXT_ATTR_CURRUPTED;
> > > > + return cmp ? ENODATA : 0;
> > > > +}
> > > > +
> > > > +errcode_t ext2fs_ext_attr_ibody_find(ext2_filsys fs,
> > > > + struct ext2_inode_large *inode,
> > > > + struct ext2_ext_attr_info *i,
> > > > + struct ext2_ext_attr_search *s)
> > > > +{
> > > > + struct ext2_ext_attr_ibody_header *header;
> > > > + errcode_t error;
> > > > +
> > > > + if (inode->i_extra_isize == 0)
> > > > + return 0;
> > > > + header = IHDR(inode);
> > > > + s->base = s->first = IFIRST(header);
> > > > + s->here = s->first;
> > > > + s->end = (char *)inode + EXT2_INODE_SIZE(fs->super);
> > > > +
> > > > + error = ext2fs_ext_attr_check_names(IFIRST(header), s->end);
> > > > + if (error)
> > > > + return error;
> > > > + /* Find the named attribute. */
> > > > + error = ext2fs_ext_attr_find_entry(&s->here, i->name_index,
> > > > + i->name, (char *)s->end -
> > > > + (char *)s->base, 0);
> > > > + if (error && error != ENODATA)
> > > > + return error;
> > > > + s->not_found = error;
> > > > + return 0;
> > >
> > > Huh. It just occurred to me (yes, after three months, sorry...) that your
> > > patchset doesn't deal with EAs that live in a separate block. For the purposes
> > > of inline_data I suppose that's sufficient, but if we're going to add an API to
> > > mess with EAs, we ought to be able to handle all of a file's EAs.
> > >
> > > The implementation that I wrote for fuse2fs does this, but it has no facility
> > > to ensure that the inline data ends up in the ibody and not the EA block.
> > > Though really, they're written out in array order, so it wouldn't be difficult
> > > to sort before writing, or use some knapsack variant.
> >
> > Sorry, I still havn't had a chance to take a closer look at your patch
> > set for e2fsprogs. I will review it ASAP.
>
> Ok. I'm getting ready to re-spam the mailing list with some more fixes, so you
> can relax for a day or two. There were some bugs handling inode_size=128.
Welcome to flush my mail box. :-p
- Zheng
>
> (If you want to take a quick look that's fine; I haven't changed the patch
> significantly.)
>
> --D
> >
> > >
> > > I think I might be drifting towards the idea of attaching your patch series to
> > > the end of mine, but as yours is already in -pu I'm open to discussing how to
> > > reconcile our implementations.
> >
> > I agree with you that we need to reconcile our implementation. I was
> > wondering if we really need to add all APIs for EAs in e2fsprogs when I
> > added these functions. But now it seems that we do need them.
> >
> > >
> > > I _do_ like the comments you added to ext2fs_ext_attr_set_entry() though. :)
> >
> > Yes, there is no any difficulty to add this API. :-)
> >
> > Regards,
> > - Zheng
> --
> 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
next prev parent reply other threads:[~2013-10-12 8:39 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-02 9:49 [PATCH v1 00/22] e2fsprogs: inline data refinement patch set Zheng Liu
2013-08-02 9:49 ` [PATCH v1 01/22] libext2fs: add INLINE_DATA into EXT2_LIB_SOFTSUPP_INCOMPAT Zheng Liu
2013-10-13 3:21 ` Theodore Ts'o
2013-08-02 9:49 ` [PATCH v1 02/22] libext2fs: add function to check inline_data flag for an inode Zheng Liu
2013-08-02 9:49 ` [PATCH v1 03/22] libext2fs: add functions to operate on extended attribute Zheng Liu
2013-08-05 17:34 ` Darrick J. Wong
2013-08-05 23:14 ` Zheng Liu
2013-10-14 1:55 ` Theodore Ts'o
2013-10-14 2:41 ` Zheng Liu
2013-10-11 22:51 ` Darrick J. Wong
2013-10-12 5:51 ` Zheng Liu
2013-10-12 8:32 ` Darrick J. Wong
2013-10-12 8:41 ` Zheng Liu [this message]
2013-10-12 9:02 ` Darrick J. Wong
2013-10-12 9:08 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 04/22] libext2fs: handle inline data in dir iterator function Zheng Liu
2013-10-11 23:33 ` Darrick J. Wong
2013-10-12 5:55 ` Zheng Liu
2013-10-13 22:51 ` Theodore Ts'o
2013-10-14 3:07 ` Zheng Liu
2013-10-14 1:58 ` Theodore Ts'o
2013-08-02 9:49 ` [PATCH v1 05/22] libext2fs: handle inline_data in block " Zheng Liu
2013-10-13 3:55 ` Theodore Ts'o
2013-10-14 0:44 ` Theodore Ts'o
2013-10-14 2:49 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 06/22] debugfs: make stat command support inline data Zheng Liu
2013-10-11 23:43 ` Darrick J. Wong
2013-10-12 0:07 ` Darrick J. Wong
2013-08-02 9:49 ` [PATCH v1 07/22] debugfs: make mkdir and expanddir " Zheng Liu
2013-10-12 0:21 ` Darrick J. Wong
2013-10-12 7:15 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 08/22] debugfs: make lsdel " Zheng Liu
2013-08-02 9:49 ` [PATCH v1 09/22] libext2fs: handle inline data in read/write function Zheng Liu
2013-08-02 9:49 ` [PATCH v1 10/22] debugfs: handle inline_data feature in dirsearch command Zheng Liu
2013-10-12 0:30 ` Darrick J. Wong
2013-10-12 7:21 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 11/22] debugfs: handle inline_data feature in bmap command Zheng Liu
2013-08-02 9:49 ` [PATCH v1 12/22] debugfs: handle inline_data in punch command Zheng Liu
2013-10-12 0:37 ` Darrick J. Wong
2013-10-12 7:22 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 13/22] libext2fs: add inline_data feature into EXT2_LIB_FEATURE_INCOMPAT_SUPP Zheng Liu
2013-08-02 9:49 ` [PATCH v1 14/22] mke2fs: add inline_data support in mke2fs Zheng Liu
2013-10-12 0:27 ` Darrick J. Wong
2013-10-12 8:08 ` Zheng Liu
2013-10-12 8:18 ` Darrick J. Wong
2013-10-12 8:31 ` Zheng Liu
2013-10-12 8:32 ` Darrick J. Wong
2013-08-02 9:49 ` [PATCH v1 15/22] tune2fs: add inline_data feature in tune2fs Zheng Liu
2013-10-12 0:39 ` Darrick J. Wong
2013-10-12 8:16 ` Zheng Liu
2013-10-12 8:23 ` Darrick J. Wong
2013-10-12 8:33 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 16/22] e2fsck: add problem descriptions and check inline data feature Zheng Liu
2013-08-02 9:49 ` [PATCH v1 17/22] e2fsck: check inline_data in pass1 Zheng Liu
2013-10-12 0:47 ` Darrick J. Wong
2013-10-12 8:17 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 18/22] e2fsck: check inline_data in pass2 Zheng Liu
2013-08-02 9:49 ` [PATCH v1 19/22] e2fsck: check inline_data in pass3 Zheng Liu
2013-10-12 0:54 ` Darrick J. Wong
2013-10-12 9:06 ` Zheng Liu
2013-10-12 9:09 ` Darrick J. Wong
2013-10-12 9:17 ` Zheng Liu
2013-10-12 9:22 ` Darrick J. Wong
2013-10-12 9:32 ` Zheng Liu
2013-08-02 9:49 ` [PATCH v1 20/22] tests: change result in f_bad_disconnected_inode Zheng Liu
2013-08-02 9:49 ` [PATCH v1 21/22] mke2fs: enable inline_data feature on ext4dev filesystem Zheng Liu
2013-08-02 9:49 ` [PATCH v1 22/22] libext2fs: add a unit test for inline data Zheng Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131012084111.GF6462@gmail.com \
--to=gnehzuil.liu@gmail.com \
--cc=darrick.wong@oracle.com \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=wenqing.lz@taobao.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).