On 04/30/2013 07:57 PM, Josh Durgin wrote: > 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. That's a good idea. And although the messenger is prepared to handle that, the osd client and rbd still need some work to really be able to handle generalized many-op requests. I can see it on the horizon, but right now we still really assume at most 2 ops, and only one of them (the first) is a read. Format 1 of course does the whole thing in one shot. The second thing I was going to say is that this change actually makes it *better* than it was. Previously we would fetch all this stuff for every snapshot on every refresh. Now (for format 2 anyway) we get only the snapshot context, and only when we're mapping do we look up more detailed snapshot info. On that, the only thing inefficient is looking up the snapshot id given its name. It is so inefficient it might be worth offering an op that does that for us, because as it is we have to do a sequential search to find out a snapshot's name. (Maybe it's not that important to track the name for parent snapshots but users might be interested.) > 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. That should be pretty easy to fix. I forgot about that. I just lopped out that whole function and I guess I missed that. > 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
