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

[David Sterba <dave@jikos.cz>]

Hi,

On Tue, Jun 14, 2011 at 03:17:47PM -0400, Josef Bacik wrote:
> Currently there is nothing protecting the pending_snapshots list on the
> transaction.  We only hold the directory mutex that we are snapshotting and a
> read lock on the subvol_sem, so we could race with somebody else creating a
> snapshot in a different directory and end up with list corruption.  So protect
> this list with the trans_lock.  Thanks,

it's correct that the pending_snapshots list is not protected, but I do
not see a way how to corrupt the list.

The callchain goes like this:

<user called ioctl>
  btrfs_ioctl_snap_create_transid
    btrfs_mksubvol
      create_snapshot

the interesting stuff happens inside create_snapshot:

* alloc pending snapshot struct
* start transaction (btrfs_start_transaction)
* reserve metadata
* list_add pending_snapshot to the transaction
* commit the transaction (sync or async)
* free pending snapshot struct

the snapshots are really created from btrfs_commit_transaction,
create_pending_snapshots(), which does not protect the
trans->pending_snapshots list, just traverses it. There is a potential
corruption too but ...

... there is the 'start transaction' call, which tries very hard to
start a new transaction, ie. this call returns a new transaction handle.
To prove this point needs more space, I will skip it now. The important
thing for our case is that new transaction waits for the current to
unblock.

Then, each snap create will have its own transaction and own
pending_snapshots list, and will create just one snapshot per
transaction, because any concurrently running ioctl snap create will block
in the start_transaction call.

Therefore I think that either

1) there is no locking needed around pending_snapshots (under assumption
one snapshot per transaction), or

2) you need to protect every access to the pending_snapshot:
- add in create_snapshot
- splice in btrfs_destroy_pending_snapshots
- list foreach in create_pending_snapshots
- list_empty in btrfs_commit_transaction


david