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?
- ext3_journal_stop() can return an error code.
- 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?
- In ext3_attr_free_inode() it doesn't appear that you are journaling
the changes to the inode itself. Are you doing this elsewhere?
- 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? If you find a bad EA
block, you should probably call ext3_error() to force fsck on the
next boot.
- ea_error() should probably just be replaced with a call to ext3_error()
or ext3_warning(), as appropriate, which would solve the problem above.
- 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.
- 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?
- 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.
- 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.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/