[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: Extended Attributes and Access Control Lists



On Sun, 28 Oct 2001, Andreas Dilger wrote:

> On Oct 28, 2001  23:03 +0100, Andreas Gruenbacher wrote:
> > Since I'm not very much into the innards of ext3, can some of you please
> > take a look at the patch, and see whether it contains any flaws (and tell
> > me which flaws)? Thanks!
> > 
> > The patch can be found at <http://acl.bestbits.at/>.
> 
> Hmm, I had a good look around, and couldn't find it.  Finally, I see
> that it is already part of the "current" patches.
> 
>   http://acl.bestbits.at/current/linux-2.4.13ea-0.7.22.patch.gz
>   http://acl.bestbits.at/current/linux-2.4.13-ac3-ea-0.7.22.patch.gz
>   http://acl.bestbits.at/current/linux-2.4.13-ac4-ea-0.7.22.patch.gz
> 
> 
> Well, as a start, it would be nice if the EA/ACL code followed the
> coding style of ext2 and ext3 (and Linus) (i.e. function declarations).
> 
> - It seems that there are not enough files modified in the ext3 tree
>   to implement EAs properly (at least compared to the number of files
>   changed in the ext2 tree.  Is part of the patch missing?

I don't think so. It's seven files modified in ext2 as well as in
ext3. Which patches are you comparing?

> - ext3_journal_stop() can return an error code.

Okay.

> - I would suggest using more specific macro names than "HDR" and "ENTRY"
> - I'm not saying that there is a problem, but it in ext3_set_ext_attr()
>   it is very hard to see whether your ext3_journal_get_write_access()
>   calls always have an ext3_journal_dirty_metadata() to go along with
>   them, and I think the original coders have the same problem (e.g. the
>   commented-out call to ext3_journal_dirty_metadata() at the end.  Is it
>   possible to split up this _huge_ function into easier-to-digest bits?

True, ext{2,3}_set_ext_attr is terrible. I didn't yet find a way to break
up the function easily.

> - In ext3_attr_free_inode() it doesn't appear that you are journaling
>   the changes to the inode itself.  Are you doing this elsewhere?

No, the caller does it (I will check whether that's still true in all
cases).

How can I properly write a change to a block, and then free it? Does this
call sequence make sense? (It's not critical when the changes are written
back; this just poisons the block in case something really strange
happens.)

    HDR(old_bh)->h_magic = 0;
    ext3_journal_dirty_metadata(handle, old_bh);
    ext3_free_blocks(handle, inode, old_bh->b_blocknr, 1);
    ext3_forget(handle, 1, inode, old_bh, old_bh->b_blocknr);

> - Where/how is ext3_ext_attr_check_block() used?  It would seem that this
>   should be part of e2fsck only.  If runtime checks are really needed,
>   could you do something like ext3_check_page() where we only check the
>   data a single time when it is read from disk?

Good observation. The additional checks are not really necessary.

>   If you find a bad EA
>   block, you should probably call ext3_error() to force fsck on the
>   next boot.

Okay.

> - In ext3_ext_attr_cmp() shouldn't it be enough to compare only the
>   hash values of each entry (or even better - only the hash value in
>   the header)?  Maybe the entry hash should include the name as well?
>   I don't think you will ever want to merge EAs with identical data
>   but different names.  It seems like we are comparing too many things.

The hash values in the header only differ if a hash collision occurs,
which hopefully is a very rare event. Therefore I'm not doing this check
at all, and directly go to the more detailed checks.

Comparing the hash values of entries alone isn't enough because of
collisons, though. Each entry hash does include the name and value.

> - Conversely, it appears that we are not comparing EA values in any
>   cardinal "order", so that if you have one block with EAs foo,bar,
>   and another block with EAs bar,foo (both with identical contents)
>   they will not merge even though they should (AFAIK, position within
>   the EA block should not be important).  Maybe comparing in EA-name
>   order is best?

Entries are always kept in a cardinal order in disk, so this problem
actually doesn't occur.

> - The mbcache stuff - you may as well move dcache, icache, dqcache
>   into the list of caches, since we seem to be getting a lot of them,
>   and it would clean up the code a bit.

At this stage I didn't want to be too intrusive. That's certainly
possible, though.

> - You should probably see if Linus will allow you to reserve the EA
>   syscall numbers, so that you don't need to change them again if he
>   adds new syscalls.  The EA stuff is mature enough (and close enough
>   to inclusion into the main kernel) that allocating syscall numbers
>   is a reasonable thing to do, even if the code is not included yet.

That request was recently denied to me and the XFS folks because we did
not yet finish negotiating a common syscall interface.


Many thanks for your comments!

--Andreas.






[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]