Hi Jan, comments inline as usual!
On Thu, Apr 12, 2012 at 07:59:36PM +0200, Jan Schmidt wrote:
> > @@ -858,62 +859,75 @@ static int inode_ref_info(u64 inum, u64 ioff, struct
> > btrfs_root *fs_root,
> > }
> >
> > /*
> > - * this iterates to turn a btrfs_inode_ref into a full filesystem path.
> > elements
> > - * of the path are separated by '/' and the path is guaranteed to be
> > - * 0-terminated. the path is only given within the current file system.
> > - * Therefore, it never starts with a '/'. the caller is responsible to
> > provide
> > - * "size" bytes in "dest". the dest buffer will be filled backwards.
> > finally,
> > - * the start point of the resulting string is returned. this pointer is
> > within
> > - * dest, normally.
> > - * in case the path buffer would overflow, the pointer is decremented
> > further
> > - * as if output was written to the buffer, though no more output is
> > actually
> > - * generated. that way, the caller can determine how much space would be
> > - * required for the path to fit into the buffer. in that case, the returned
> > - * value will be smaller than dest. callers must check this!
> > + * Given the parent objectid and name/name_len pairs of an inode ref
> > + * (any version) this iterates to turn that information into a
> > + * full filesystem path. elements of the path are separated by '/' and
> > + * the path is guaranteed to be 0-terminated. the path is only given
> > + * within the current file system. Therefore, it never starts with a
> > + * '/'. the caller is responsible to provide "size" bytes in
> > + * "dest". the dest buffer will be filled backwards. finally, the
> > + * start point of the resulting string is returned. this pointer is
> > + * within dest, normally. in case the path buffer would overflow, the
> > + * pointer is decremented further as if output was written to the
> > + * buffer, though no more output is actually generated. that way, the
> > + * caller can determine how much space would be required for the path
> > + * to fit into the buffer. in that case, the returned value will be
> > + * smaller than dest. callers must check this!
>
> It would reduce patch sets if you can extend comments in a compatible
> way, you make reviewers happy if you don't realign text (or, later,
> function parameters) where it's not required.
Yeah I just reverted the comment change as it's no longer needed anyway.
> > */
> > static char *iref_to_path(struct btrfs_root *fs_root, struct btrfs_path
> > *path,
> > - struct btrfs_inode_ref *iref,
> > - struct extent_buffer *eb_in, u64 parent,
> > - char *dest, u32 size)
> > + int name_len, unsigned long name_off,
>
> name_len should be u32
hmm ok that's fine.
> > + struct extent_buffer *eb_in, u64 parent,
> > + char *dest, u32 size)
> > {
> > - u32 len;
> > int slot;
> > u64 next_inum;
> > int ret;
> > s64 bytes_left = size - 1;
> > struct extent_buffer *eb = eb_in;
> > struct btrfs_key found_key;
> > + struct btrfs_inode_ref *iref;
> > + struct btrfs_inode_extref *iref2;
>
> iextref
done.
> > if (bytes_left >= 0)
> > dest[bytes_left] = '\0';
> >
> > while (1) {
> > - len = btrfs_inode_ref_name_len(eb, iref);
> > - bytes_left -= len;
> > + bytes_left -= name_len;
> > if (bytes_left >= 0)
> > read_extent_buffer(eb, dest + bytes_left,
> > - (unsigned long)(iref + 1), len);
> > + name_off, name_len);
> > if (eb != eb_in)
> > free_extent_buffer(eb);
> > +
> > + /* Ok, we have enough to find any refs to the parent inode. */
> > ret = inode_ref_info(parent, 0, fs_root, path, &found_key);
> > - if (ret > 0)
> > - ret = -ENOENT;
> > - if (ret)
> > - break;
> > next_inum = found_key.offset;
> > + if (ret == 0) {
> > + slot = path->slots[0];
> > + eb = path->nodes[0];
> > + /* make sure we can use eb after releasing the path */
> > + if (eb != eb_in)
> > + atomic_inc(&eb->refs);
> > + btrfs_release_path(path);
> > + iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> > +
> > + name_len = btrfs_inode_ref_name_len(eb, iref);
> > + name_off = (unsigned long)(iref + 1);
> > + } else {
> > + ret = btrfs_find_one_extref(fs_root, parent, 0, path,
> > + &iref2, NULL);
> > + if (ret)
> > + break;
> > +
> > + next_inum = btrfs_inode_extref_parent(eb, iref2);
> > + name_off = (unsigned long)&iref2->name;
> > + name_len = btrfs_inode_extref_name_len(eb, iref2);
> > + }
> >
> > /* regular exit ahead */
> > if (parent == next_inum)
> > break;
>
> These regular exit lines must go before the block you inserted.
> Otherwise we leak a reference on eb if it's != eb_in.
Good catch, fixed by virtue of fixing the below issue :)
> Whereas I think we don't need this if-else-construct at all. We do need
> the changes you made as to passing name_len and name_off, I agree.
> However, the rest of the function should stay as it was, because the
> parent of each object must be a directory and a directory won't have
> hard links. Thus, we'll never meet INODE_EXTREFs when walking up the
> path. Or did I miss something?
You didn't miss anything, at the time I coded this part I wasn't 100% sure
if we would use inode refs on a directory for something *other* than
hardlinking a dir (like, possibly some internal use I wasn't aware of). I'm
pretty confident now that this can't happen so I think the best approach is
to just kill the code that's handling this condition and leave a nice
comment.
> >
> > - slot = path->slots[0];
> > - eb = path->nodes[0];
> > - /* make sure we can use eb after releasing the path */
> > - if (eb != eb_in)
> > - atomic_inc(&eb->refs);
> > - btrfs_release_path(path);
> > -
> > - iref = btrfs_item_ptr(eb, slot, struct btrfs_inode_ref);
> > parent = next_inum;
> > --bytes_left;
> > if (bytes_left >= 0)
> > @@ -1226,9 +1240,9 @@ int iterate_inodes_from_logical(u64 logical, struct
> > btrfs_fs_info *fs_info,
> > return ret;
> > }
> >
> > -static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
> > - struct btrfs_path *path,
> > - iterate_irefs_t *iterate, void *ctx)
> > +static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
> > + struct btrfs_path *path,
> > + iterate_irefs_t *iterate, void *ctx)
>
> This function must not call free_extent_buffer(eb) in line 1306 after
> applying your patch set (immediately before the break). Second, I think
> we'd better add a blocking read lock on eb after incrementing it's
> refcount, because we need the current content to stay as it is. Both
> isn't part of your patches, but it might be easier if you make that
> bugfix change as a 3/4 patch within your set and turn this one into 4/4.
> If you don't like that, I'll send a separate patch for it. Don't miss
> the unlock if you do it ;-)
Ok, I think I was able to figure out and add the correct locking calls.
Basically I believe I need to wrap access around:
btrfs_tree_read_lock(eb);
btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
<read eb contents>
btrfs_tree_read_unlock_blocking(eb);
> > {
> > int ret;
> > int slot;
> > @@ -1244,7 +1258,7 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> > *fs_root,
> >
> > while (1) {
> > ret = inode_ref_info(inum, parent ? parent+1 : 0, fs_root, path,
> > - &found_key);
> > + &found_key);
> > if (ret < 0)
> > break;
> > if (ret) {
> > @@ -1286,6 +1300,76 @@ static int iterate_irefs(u64 inum, struct btrfs_root
> > *fs_root,
> > return ret;
> > }
> >
> > +static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
> > + struct btrfs_path *path,
> > + iterate_extrefs_t *iterate, void *ctx)
> > +{
> > + int ret;
> > + int slot;
> > + u64 offset = 0;
> > + u64 parent;
> > + int found = 0;
> > + struct extent_buffer *eb;
> > + struct btrfs_item *item;
> > + struct btrfs_inode_extref *iref2;
>
> iextref
Done.
> > +
> > + while (1) {
> > + ret = btrfs_find_one_extref(fs_root, inum, offset, path, &iref2,
> > + &offset);
> > + if (ret < 0)
> > + break;
> > + if (ret) {
> > + ret = found ? 0 : -ENOENT;
> > + break;
> > + }
> > + ++found;
> > +
> > + slot = path->slots[0];
> > + eb = path->nodes[0];
> > + /* make sure we can use eb after releasing the path */
> > + atomic_inc(&eb->refs);
>
> You need a blocking read lock here, too. Grab it before releasing the path.
Done.
>
> > + btrfs_release_path(path);
> > +
> > + item = btrfs_item_nr(eb, slot);
>
> You don't need item.
Removed.
>
> > + iref2 = btrfs_item_ptr(eb, slot, struct btrfs_inode_extref);
> > +
> > + parent = btrfs_inode_extref_parent(eb, iref2);
> > + ret = iterate(parent, iref2, eb, ctx);
>
> The caller shouldn't have to deal with two different types of callbacks.
> Please just build a dummy struct btrfs_inode_ref object here and pass it
> to iterate.
> Alternatively, you can extract the information the caller
> will need, here, and pass that instead of a struct btrfs_inode_ref. This
> way, we can use the same type for both iterate() functions.
Ask and you shall receive :)
It turns out to be pretty trivial to just pass in name_len and the name
pointer as that's all those callbacks needed from the ref.
> > + if (ret) {
> > + free_extent_buffer(eb);
> > + break;
> > + }
> > +
> > + free_extent_buffer(eb);
>
> Call free_extent_buffer(eb) before the if (ret), drop it from the if
> block, add an unlock before the if block.
Fixed that.
> > + offset++;
>
> Another caller not expecting btrfs_find_one_extref to return offset
> incremented. The offset++ should stay here and btrfs_find_one_extref
> should just return the plain offset.
>
> > + }
> > +
> > + btrfs_release_path(path);
> > +
> > + return ret;
> > +}
> > +
> > +static int iterate_irefs(u64 inum, struct btrfs_root *fs_root,
> > + struct btrfs_path *path,
> > + iterate_irefs_t *iterate,
> > + iterate_extrefs_t *iterate2, void *ctx)
>
> As mentioned above, I'd like to see only be a single iterate function at
> this level.
>
> > +{
> > + int ret, found_refs = 0;
>
> split
done.
> > +
> > + ret = iterate_inode_refs(inum, fs_root, path, iterate, ctx);
> > + if (ret && ret != -ENOENT)
> > + return ret;
> > +
> > + if (ret != -ENOENT)
> > + ++found_refs;
>
> I'd make those 2 if statements:
>
> if (!ret)
> ++found_refs;
> else if (ret != -ENOENT)
> return ret;
done.
> > +
> > +/*
> > + * returns 0 if the path could be dumped (probably truncated)
> > + * returns <0 in case of an error
> > + */
> > +static int inode_to_path2(u64 inum, struct btrfs_inode_extref *iref2,
> > + struct extent_buffer *eb, void *ctx)
>
> We'll get rid of a second inode_to_path, too, if my suggestion works out.
Yep.
Thanks Jan!
--Mark
--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
