On 09/17/2013 10:24 AM, Josh Durgin wrote: >> >> As long as you're testing... >> >> It would be good to demonstrate that it's writable after >> a setrw call (i.e., by actually writing to it). >> >> And it would be good to show an attempt to change to >> read-write a mapped "base" image as well as a mapped >> snapshot image, both opened (mounted) and not. > > Also a read-only mapped non-snapshot, which will catch > a bug with this patch: > > rbd_request_fn reads rbd_dev->mapping.read_only when it is first > called, but once it's in the processing loop it never checks it again. > This will prevent an initially read-only mapping from ever becoming > read-write. The request loop needs to check for an updated > rbd_dev->mapping.read_only value. You are absolutely right. The existing code assumes it won't change, basically. > This makes me notice another cleanup, though it doesn't affect the > functionality in this patch: > > The place where the rbd driver calls set_device_ro() should be changed > as well - it only needs to be done once, when the device is being added, > not during each open() call. Yes, probably in rbd_dev_device_setup(), near the set_capacity() call. Very good finds Josh. -Alex -- 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
