On Thursday, October 11, 2001 01:57:15 PM -0600 Andreas Dilger <adilger@turbolabs.com> wrote: > I was looking at this, and thought the following: > > 1) Shouldn't we just initialize r=1 in _remap_snapshot(), and then if > lv->lv_block_exception is NULL we will not enter __remap_snapshot() > at all? That would remove the need to get the write semaphore for > no reason. Good point. > 2) Initially, I thought we could just "optimize" the checking of > lv->lv_block_exception, but I supposed there are race conditions > in dropping the read lock and getting the write lock, so I guess > we still need to check lv->lv_block_exception in __remap_snapshot() > also? Yes, I think we do. > 3) The alternative would be to check lv->lv_block_exception inside > lvm_snapshot_remap_block() instead of in the callers (returning "1" > if it is NULL, and we don't want to do remap). This would avoid any > problems in the future if someone else calls lvm_snapshot_remap_block() > without checking lv_block_exception first. Untested patch below which > should be equivalent to your previous patch. The only thing I don't like about this is that it makes it hides the fact that lvm_snapshot_COW and a few other calls depend on block_exception being valid. The code certainly looks right though, just a matter of style. thanks, Chris
