On Tue, May 17, 2016 at 03:20:16PM +0200, David Sterba wrote:
> On Fri, Apr 01, 2016 at 02:35:01PM +0800, Qu Wenruo wrote:
> > @@ -5815,6 +5817,23 @@ out_fail:
> > }
> > if (delalloc_lock)
> > mutex_unlock(&BTRFS_I(inode)->delalloc_mutex);
> > + /*
> > + * The number of metadata bytes is calculated by the difference
> > + * between outstanding_extents and reserved_extents. Sometimes though
> > + * reserve_metadata_bytes() fails to reserve the wanted metadata bytes,
> > + * indeed it has already done some work to reclaim metadata space, hence
> > + * both outstanding_extents and reserved_extents would have changed and
> > + * the bytes we try to reserve would also has changed(may be smaller).
> > + * So here we try to reserve again. This is much useful for online
> > + * dedupe, which will easily eat almost all meta space.
> > + *
> > + * XXX: Indeed here 3 is arbitrarily choosed, it's a good workaround for
> > + * online dedupe, later we should find a better method to avoid dedupe
> > + * enospc issue.
> > + */
> > + if (unlikely(ret == -ENOSPC && loops++ < 3))
> > + goto again;
> > +
>
> This does not seem right and needs to be addressed properly before I
> consider adding the patchset to for-next. I don't have idea how to fix
> it.
Agreed, and this sort of issue is a reason why I strongly feel we don't want
to merge this series piecemeal until we know that after everything is
complete, we can end up with a fully baked in-band dedupe implementation.
Luckily Qu says he's on it so if he posts a workable fix here my whole point
can become moot. Until then though this is exactly the type of 'fix later'
coding we need to be avoiding.
--Mark
--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
