Re: 3.7.0-rc8 btrfs locking issue

[Liu Bo <bo.li.liu@oracle.com>]

On Wed, Dec 12, 2012 at 09:37:37AM +0800, Liu Bo wrote:
> On Tue, Dec 11, 2012 at 09:33:15AM -0700, Jim Schutt wrote:
> > On 12/09/2012 07:04 AM, Liu Bo wrote:
> > > On Wed, Dec 05, 2012 at 09:07:05AM -0700, Jim Schutt wrote:
> > > Hi Jim,
> > > 
> > > Could you please apply the following patch to test if it works?
> > 
> > Hi,
> > 
> > So far, with your patch applied I've been unable to reproduce
> > the recursive deadlock.  Thanks a lot for this patch!
> > This issue has been troubling me for a while.
> 
> Hi Jim,
> 
> Good news for us :)
> 
> > 
> > I've been trying to learn more about btrfs internals -
> > if you have the time to answer a couple questions about
> > your patch, I'd really appreciate it.
> 
> See below.
> 
> > 
> > > 
> > > (It's against 3.7-rc8.)
> > > 
> > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > > index 3d3e2c1..100289b 100644
> > > --- a/fs/btrfs/extent-tree.c
> > > +++ b/fs/btrfs/extent-tree.c
> > > @@ -3346,7 +3346,8 @@ u64 btrfs_get_alloc_profile(struct btrfs_root
> > > *root, int data)
> > >  
> > >  	if (data)
> > >  		flags = BTRFS_BLOCK_GROUP_DATA;
> > > -	else if (root == root->fs_info->chunk_root)
> > > +	else if (root == root->fs_info->chunk_root ||
> > > +		 root == root->fs_info->dev_root)
> > >  		flags = BTRFS_BLOCK_GROUP_SYSTEM;
> > >  	else
> > >  		flags = BTRFS_BLOCK_GROUP_METADATA;
> > > @@ -3535,6 +3536,7 @@ static u64 get_system_chunk_thresh(struct
> > > btrfs_root *root, u64 type)
> > >  		num_dev = 1;	/* DUP or single */
> > >  
> > >  	/* metadata for updaing devices and chunk tree */
> > > +	num_dev = num_dev << 1
> > 
> > AFAICS this is doubling the size of the reserve, which
> > reduces the chance of a recursive do_chunk_alloc(), right?
> > 
> 
> Not like that, we hit the deadlock because updating device tree also
> uses METADATA chunk, which may be called when we're actually allocating
> a METADATA chunk, so the patch I sent you makes updating device tree
> use SYSTEM chunk, which we'll have some code to check if it is enough
> before allocating a chunk(if not, we'll allocate a SYSTEM chunk first).
> 
> Here I double the size just because the worst case of allocating a
> DATA/METADATA chunk -may- results in
> 
> 1)adding a SYSTEM chunk +
> 2)adding dev extent per chunk stripe +
> 3)updating chunk stripes's bytes_used
> 
> > >  	return btrfs_calc_trans_metadata_size(root, num_dev + 1);
> > 
> > btrfs_calc_trans_metadata_size(root, num_items) multiplies its
> > num_items argument by another factor of three - do you know if
> > there is there some rationale behind that number, or is it
> > perhaps also an empirically determined factor?
> 
> The height of Btrfs's metadata btree is at most 8,
> leaf is on 0 level while node is at most on 7 level.
> 
> Each btree update may results in COWing a node and its sibling nodes,
> where the factor of tree comes from

s/tree/three/g

> 
> > 
> > What I'm wondering about is that if the size of the reserve is
> > empirically determined, will it need to be increased again
> > later when machines are more capable, and can handle a higher
> > load?
> > 
> > Do you think it's feasible to modify the locking for
> > do_chunk_alloc to allow it to recurse without deadlock?
> 
> Well, it could be, but IMO it'll bring us complexity, so worse
> maintainance.
> 
> Any questions? Feel free to ask.
> 
> thanks,
> liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/