On Thu, Jun 27, 2013 at 02:33:20PM -0700, Zach Brown wrote:
> Reviewing as requested! It certainly looks reasonable, but I don't have
> enough history with the code to really say much more than that.
>
> Some questions:
>
> > @@ -8423,6 +8432,10 @@ void btrfs_create_pending_block_groups(struct
> > btrfs_trans_handle *trans,
> > sizeof(item));
> > if (ret)
> > btrfs_abort_transaction(trans, extent_root, ret);
> > + ret = btrfs_finish_chunk_alloc(trans, extent_root,
> > + key.objectid, key.offset);
>
> Would this moved call site need to worry about the chunk mutex at all?
>
No, we're just using the chunk_mutex to serialize the allocations. So since all
the stuff we care about is in memory or in the commit root we only need to have
the chunk_mutex around the actuall allocation part.
> > + if (ret)
> > + btrfs_abort_transaction(trans, root, ret);
>
> Did you mean to abort the root here, not the extent_root like the one
> above?
I'm not sure it matters but I'll change it for consistency sake.
>
> > +static int contains_pending_extent(struct btrfs_trans_handle *trans,
> > + struct btrfs_device *device,
> > + u64 *start, u64 len)
> > +{
> > + struct extent_map *em;
> > + int ret = 0;
> > +
> > + list_for_each_entry(em, &trans->transaction->pending_chunks, list) {
> > + struct map_lookup *map;
> > + int i;
> > +
> > + map = (struct map_lookup *)em->bdev;
> > + for (i = 0; i < map->num_stripes; i++) {
> > + if (map->stripes[i].dev != device)
> > + continue;
> > + if (map->stripes[i].physical >= *start + len ||
> > + map->stripes[i].physical + em->orig_block_len <=
> > + *start)
> > + continue;
> > + *start = map->stripes[i].physical +
> > + em->orig_block_len;
> > + ret = 1;
> > + }
> > + }
>
> Did you want a 'break;' there? Is it trying to set *start to the last
> match rather than returning the first?
>
We want the last, otherwise we'll set it to the end of the first extent we find,
go back and do all that searching again, come back here and just adjust it up
again if theres another allocation sitting there that butts up to the first
entry we found.
> > + ret = btrfs_alloc_dev_extent(trans, device,
> > + chunk_root->root_key.objectid,
> > + BTRFS_FIRST_CHUNK_TREE_OBJECTID,
> > + chunk_offset, dev_offset,
> > + stripe_size);
> > + if (ret)
> > + goto out;
>
> The old code tried to free up previous allocations when one failed, but
> it looks like this deferred alloc doesn't. Do we care?
>
So before we could back out of having a chunk allocation at that point because
we hadn't let go of the chunk mutex and nobody had been able to allocate from
that chunk. With the new code if we fail here we have to abort because we could
have made allocations in our new chunk. It is kind of a drawback, but really if
this part failed before it was likely because of -EIO which we would likely hit
somewhere else in the stack and end up aborted anyway, so no big loss I think?
> > - index = 0;
> > - while (index < map->num_stripes) {
> > - index++;
>
> > + for (i = 0; i < map->num_stripes; i++) {
>
> *hugs*
>
Thanks for the review! I'll just fix up that one inconsistency and then commit
and push to btrfs-next instead of reposting. Thanks,
Josef
--
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
