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@xxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
