Please don't reply to lustre-devel. Instead, comment in Bugzilla by using the
following link:
https://bugzilla.lustre.org/show_bug.cgi?id=11510
What |Removed |Added
----------------------------------------------------------------------------
Attachment #9525|review?([EMAIL PROTECTED]|review-
Flag|m) |
(From update of attachment 9525)
>-errcode_t ext2fs_extent_split(struct ext3_extent_header *eh,
>+errcode_t ext2fs_extent_split2(struct ext3_extent_header *eh,
> struct ext3_extent *ex, int count)
Is this function intended to be exported for external callers? I'd think this
should be an internal helper function only, and should be declared static and
be given a better name like "ext2fs_extent_split_internal()" or similar.
>+errcode_t ext2fs_extent_split(ext2_filsys fs,
>+ struct ext3_extent_header **eh,
>+ struct ext3_extent **ex, int count, int *flag)
>+{
>+ struct ext3_extent_header *eh_l0;
>+ struct ext3_extent_header *eh_l1 = *eh;
>+ struct ext3_extent *ex_l0;
I find the names "eh_10" and "eh_11" not very informative. What do "10" and
"11" mean? Could you pick some more descriptive names like "eh_orig" and
"eh_parent" or something like that? Same for ex_10. Please also put
declarations of the same type on the same line.
>+ int entry = *ex - EXT_FIRST_EXTENT(eh_l1);
>+ int retval;
>+ blk_t new_block;
>+ char *buf;
>+ struct ext3_extent_idx *ei = EXT_FIRST_INDEX(eh_l1);
These should all be declared in the lowest scope in which they are used.
>+ if (entry < 0 || entry > eh_l1->eh_entries)
>+ return EXT2_ET_EXTENT_LEAF_BAD;
Too much indenting.
>+ memcpy(buf, eh_l1, eh_l1->eh_entries *
>+ sizeof(struct ext3_extent) +
>+ sizeof(struct ext3_extent_header));
A minor note - I think this would be a bit clearer if you had:
memcpy(buf, eh_11, sizeof(*eh_11) +
eh_11->eh_entries * sizeof(*ex_10));
Firstly, it is better because the eh struct is first in the array, and the
ex structs follow that. Secondly, using "sizeof(*eh)" instead of
"sizeof(struct ext3_extent_header)" is shorter and also avoids problems if
the variable type changes.
>+ eh_l0 = (struct ext3_extent_header *)buf;
>+
>+ printf("eh_l0 = %x\n", eh_l0);
>+ printf("eh_l1 = %x\n", eh_l1);
How about using show_header from EXT_DEBUG here, so we don't have to worry
about removing these later?
>+ eh_l0->eh_max = (fs->blocksize -
>+ sizeof(struct ext3_extent_header)) /
>+ sizeof(struct ext3_extent);
>+ eh_l0->eh_depth = 0;
Shouldn't the depth already be zero?
>+ ext2fs_new_block(fs, 0, 0, &new_block);
Need to check error code. Also, the block should probably be allocated right
after the buffer is, to avoid doing work if this fails. Also, please supply a
goal block to at least try and get decent allocation for the index. You could
use the ex->ee_block as the goal, though if we had the inode block number that
would be even better.
>+ *eh = eh_l0;
>+ *ex = EXT_FIRST_EXTENT(eh_l0) +
>+ sizeof(struct ext3_extent) * entry;
>+
>+ *flag = 1;
What does that flag mean? Is it BLOCK_CHANGED, or something else? Should we
also be setting BLOCK_ERROR in the error cases?
>+ return ext2fs_extent_split2(eh_l0, *ex, count);
>+
Please do not add whitespace at the end of the line, like above.
>@@ -259,14 +321,22 @@
>
> } else if (j > 0 && /* implies ex->ee_len > 1 */
> (ctx->errcode =
>- ext2fs_extent_split(eh, ex++, j))) {
>+ ext2fs_extent_split(ctx->fs, &eh,
>+ &ex, j,
>+ &extent_split_flag)
>+ /* ex++ doesnt affect the
>+ conditional statement
>+ */
Please put "*/" on previous line.
>@@ -281,11 +351,25 @@
> }
> /* FIXME: 48-bit */
> ex->ee_start = block_address;
>+
>+
>+
> ret |= BLOCK_CHANGED;
Why are these blank lines added?
>+ /* Multi level split at depth == 0.
>+ ex has been changed to point to newly allocated block
>+ buffer. And after returning in this scenario, only inode is
>+ updated with changed i_block. Hence to explicitly write to
>+ block requried.
>+ */
>+ if(extent_split_flag == 1) {
>+ struct ext3_extent_idx *ix = EXT_FIRST_INDEX(orig_eh);
>+ ctx->errcode = ext2fs_write_ext_block(ctx->fs,
>+ ix->ei_leaf,
>eh);
>+ }
I'm not sure I understand this. Didn't ext2fs_extent_split() already write the
ei_leaf == new_block? Should this instead be writing out the inode? Would it
be enough to return BLOCK_CHANGED in this case to have the caller write out the
current block?
With the exception of the above issue, I think it only needs a bit of code
cleanup before it can land into the CFS e2fsprogs patch series.
_______________________________________________
Lustre-devel mailing list
[email protected]
https://mail.clusterfs.com/mailman/listinfo/lustre-devel
