Re: [PATCH v2] rbd: add ioctl for rbd

[Alex Elder <alex.elder@linaro.org>]

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