Re: [PATCH] Btrfs: protect the pending_snapshots list with trans_lock

[David Sterba <dave@jikos.cz>]

On Wed, Jun 15, 2011 at 07:41:37AM -0400, Chris Mason wrote:
> There is definitely a window where two procs can be inside
> create_snapshot() at the same time in the same transaction.

I trust you on that. I was trying to follow the callgraph from
btrfs_strat_transaction called from create_snapshot to see whether and
how two calls may access the list and came to conclusion that there must
be a fresh transaction after the call. There are lots of
join_transaction, commit_transaction calls and I could have lost track.
I'm leaving this to me as a homework, it was very educative anyway.

> Going down to your list:
> 
> > 
> > 2) you need to protect every access to the pending_snapshot:
> > - add in create_snapshot
> 
> create_snapshot is racey.

agreed, the patch fixes it

> 
> > - splice in btrfs_destroy_pending_snapshots
> 
> This can only happen during unmount while there are no writers.
>
> > - list foreach in create_pending_snapshots
> 
> This can only happen during commit while there are no writers

this counts as protection, for both

> > - list_empty in btrfs_commit_transaction
> 
> This is there to make sure that any delayed allocation from the source
> subvol is actually done in the snapshot.  You're right that we should
> have a better check here.  The worst possible case in getting this wrong
> is that the snapshot misses some recent file writes in the original, but
> it will be completely consistent.

I think this is ok and acceptable.


Thanks,
david