Re: [PATCH 0/4] rbd: get rid of the snapshot list

[Josh Durgin <josh.durgin@inktank.com>]

On 04/30/2013 05:41 AM, Alex Elder wrote:
> An rbd device structure maintains a list of snapshot
> structures whose purpose is to cache the name, size,
> and features associated with a snapshot id.  The main
> reason it was needed was related to the presence of
> Linux device information for snapshots, which we
> no longer have.  We can look up the name, etc. "on
> the fly" about as easily as we can using the list,
> and getting rid of this list means we can eliminate
> a substantial bit of code.
>
> The final patch in this series gets rid of the snapshot
> list and the rbd_snap structure.  The first three put
> in place replacement functionality that doesn't require
> the list.
>
> 					-Alex
>
> [PATCH 1/4] rbd: look up snapshot name in names buffer
> [PATCH 2/4] rbd: use snap_id not index to look up snap info
> [PATCH 3/4] rbd: define rbd_snap_size() and rbd_snap_features()
> [PATCH 4/4] rbd: kill off the snapshot list

These patches look good, but bring up two things that should be 
addressed in later patches.

1) inefficient snapshot id lookup for format 2 images

We're sending a single request for each snapshot id in the snapshot
context. There could potentially be a very large number of snapshots
there, hundreds or even thousands, which would slow down mapping
a snapshot quite a bit. Aggregating this into a request with multiple
ops so we get, perhaps, up to 500 snapshot names at a time would
make this a lot faster.

2) mapped snapshots never clear the EXISTS flag

Now that we're not keeping a snapshot list, we don't check whether
the snapshot we mapped continues to exist. Since we know the snapshot
id, we just need to see if it's still present in the updated snapshot
context.

These aren't critical though, so this patch series looks good as is:
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>