On Tue, Aug 27, 2013 at 01:26:36PM +0300, Alex Lyakas wrote:
> Hi Josef,
> thanks for addressing this.
>
> On Mon, Aug 5, 2013 at 6:19 PM, Josef Bacik <jbacik@xxxxxxxxxxxx> wrote:
> > Alex Lyakas reported a bug where wait_block_group_cache_progress() would wait
> > forever if a drive failed. This is because we just bail out if there is an
> > error while trying to cache a block group, we don't update anybody who may be
> > waiting. So this introduces a new enum for the cache state in case of error and
> > makes everybody bail out if we have an error. Alex tested and verified this
> > patch fixed his problem. This fixes bz 59431. Thanks,
> >
> > Signed-off-by: Josef Bacik <jbacik@xxxxxxxxxxxx>
> > ---
> > fs/btrfs/ctree.h | 1 +
> > fs/btrfs/extent-tree.c | 27 ++++++++++++++++++++-------
> > 2 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index cbb1263..c17acbc 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -1188,6 +1188,7 @@ enum btrfs_caching_type {
> > BTRFS_CACHE_STARTED = 1,
> > BTRFS_CACHE_FAST = 2,
> > BTRFS_CACHE_FINISHED = 3,
> > + BTRFS_CACHE_ERROR = 4,
> > };
> >
> > enum btrfs_disk_cache_state {
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index e868c35..e6dfa7f 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -113,7 +113,8 @@ static noinline int
> > block_group_cache_done(struct btrfs_block_group_cache *cache)
> > {
> > smp_mb();
> > - return cache->cached == BTRFS_CACHE_FINISHED;
> > + return cache->cached == BTRFS_CACHE_FINISHED ||
> > + cache->cached == BTRFS_CACHE_ERROR;
> > }
> >
> > static int block_group_bits(struct btrfs_block_group_cache *cache, u64 bits)
> > @@ -389,7 +390,7 @@ static noinline void caching_thread(struct btrfs_work *work)
> > u64 total_found = 0;
> > u64 last = 0;
> > u32 nritems;
> > - int ret = 0;
> > + int ret = -ENOMEM;
> >
> > caching_ctl = container_of(work, struct btrfs_caching_control, work);
> > block_group = caching_ctl->block_group;
> > @@ -517,6 +518,12 @@ err:
> >
> > mutex_unlock(&caching_ctl->mutex);
> > out:
> > + if (ret) {
> > + spin_lock(&block_group->lock);
> > + block_group->caching_ctl = NULL;
> > + block_group->cached = BTRFS_CACHE_ERROR;
> > + spin_unlock(&block_group->lock);
> > + }
> > wake_up(&caching_ctl->wait);
> >
> > put_caching_control(caching_ctl);
> > @@ -6035,8 +6042,11 @@ static u64 stripe_align(struct btrfs_root *root,
> > * for our min num_bytes. Another option is to have it go ahead
> > * and look in the rbtree for a free extent of a given size, but this
> > * is a good start.
> > + *
> > + * Callers of this must check if cache->cached == BTRFS_CACHE_ERROR before using
> > + * any of the information in this block group.
> > */
> > -static noinline int
> > +static noinline void
> > wait_block_group_cache_progress(struct btrfs_block_group_cache *cache,
> > u64 num_bytes)
> > {
> > @@ -6044,28 +6054,29 @@ wait_block_group_cache_progress(struct btrfs_block_group_cache *cache,
> >
> > caching_ctl = get_caching_control(cache);
> > if (!caching_ctl)
> > - return 0;
> > + return;
> >
> > wait_event(caching_ctl->wait, block_group_cache_done(cache) ||
> > (cache->free_space_ctl->free_space >= num_bytes));
> >
> > put_caching_control(caching_ctl);
> > - return 0;
> > }
> >
> > static noinline int
> > wait_block_group_cache_done(struct btrfs_block_group_cache *cache)
> > {
> > struct btrfs_caching_control *caching_ctl;
> > + int ret = 0;
> >
> > caching_ctl = get_caching_control(cache);
> > if (!caching_ctl)
> > return 0;
> In case caching_thread completes with error for this block group,
> get_caching_control() will return NULL.
> So this function will return success, although the block group was not
> cached properly.
> Currently only btrfs_trim_fs() caller checks the return value of this
> function, although you didn't post the btrfs_trim_fs() change in this
> patch (but you posed it in the bugzilla). Still, should we check the
> cache->cached for ERROR even if there is no caching control?
>
Yeah I'll fix that up, sorry about that. Thanks,
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
