[PATCH] rbd: kill rbd_init_watch_dev()

[PATCH] rbd: kill rbd_init_watch_dev()

[Alex Elder]

When an rbd image is mapped, a watch request is issued so that the
client will get notified in the event the image header object
changes.  This is done using rbd_init_watch_dev(), which calls
rbd_req_sync_watch().

rbd_init_watch_dev() is organized as a loop, arranging to re-issue
the request after refreshing the header if -ERANGE ever gets
returned.  But the only way rbd_req_sync_watch() will return -ERANGE
is if the CEPH_OSD_OP_WATCH operation returns that, which it will
not.

As a result, the whole looping structure and in fact the whole
function becomes excessive.  So get rid of rbd_init_watch_dev(),
and call rbd_req_sync_watch() directly in the one place it's used.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 94d0745..71e3f3b 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2191,22 +2191,6 @@ static void rbd_bus_del_dev(struct rbd_device
*rbd_dev)
 	device_unregister(&rbd_dev->dev);
 }

-static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
-{
-	int ret, rc;
-
-	do {
-		ret = rbd_req_sync_watch(rbd_dev);
-		if (ret == -ERANGE) {
-			rc = rbd_refresh_header(rbd_dev, NULL);
-			if (rc < 0)
-				return rc;
-		}
-	} while (ret == -ERANGE);
-
-	return ret;
-}
-
 static atomic64_t rbd_id_max = ATOMIC64_INIT(0);

 /*
@@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus,
 	if (rc)
 		goto err_out_bus;

-	rc = rbd_init_watch_dev(rbd_dev);
+	rc = rbd_req_sync_watch(rbd_dev);
 	if (rc)
 		goto err_out_bus;

-- 
1.7.9.5

Re: [PATCH] rbd: kill rbd_init_watch_dev()

[Josh Durgin]

On 07/26/2012 12:11 PM, Alex Elder wrote:
> When an rbd image is mapped, a watch request is issued so that the
> client will get notified in the event the image header object
> changes.  This is done using rbd_init_watch_dev(), which calls
> rbd_req_sync_watch().
>
> rbd_init_watch_dev() is organized as a loop, arranging to re-issue
> the request after refreshing the header if -ERANGE ever gets
> returned.  But the only way rbd_req_sync_watch() will return -ERANGE
> is if the CEPH_OSD_OP_WATCH operation returns that, which it will
> not.
>
> As a result, the whole looping structure and in fact the whole
> function becomes excessive.  So get rid of rbd_init_watch_dev(),
> and call rbd_req_sync_watch() directly in the one place it's used.

Instead of getting rid of this loop, we should make it
send a compound operation consisting of (CEPH_OSD_OP_ASSERT_VER,
CEPH_OSD_OP_WATCH). Then we will get -ERANGE if our version
of the header is out of data, so we don't lose header updates
before the watch is established.

Josh

> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   18 +-----------------
>   1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 94d0745..71e3f3b 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2191,22 +2191,6 @@ static void rbd_bus_del_dev(struct rbd_device
> *rbd_dev)
>   	device_unregister(&rbd_dev->dev);
>   }
>
> -static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
> -{
> -	int ret, rc;
> -
> -	do {
> -		ret = rbd_req_sync_watch(rbd_dev);
> -		if (ret == -ERANGE) {
> -			rc = rbd_refresh_header(rbd_dev, NULL);
> -			if (rc < 0)
> -				return rc;
> -		}
> -	} while (ret == -ERANGE);
> -
> -	return ret;
> -}
> -
>   static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
>
>   /*
> @@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>   	if (rc)
>   		goto err_out_bus;
>
> -	rc = rbd_init_watch_dev(rbd_dev);
> +	rc = rbd_req_sync_watch(rbd_dev);
>   	if (rc)
>   		goto err_out_bus;
>

Re: [PATCH] rbd: kill rbd_init_watch_dev()

[Alex Elder]

On 07/30/2012 04:46 PM, Josh Durgin wrote:
> On 07/26/2012 12:11 PM, Alex Elder wrote:
>> When an rbd image is mapped, a watch request is issued so that the
>> client will get notified in the event the image header object
>> changes.  This is done using rbd_init_watch_dev(), which calls
>> rbd_req_sync_watch().
>>
>> rbd_init_watch_dev() is organized as a loop, arranging to re-issue
>> the request after refreshing the header if -ERANGE ever gets
>> returned.  But the only way rbd_req_sync_watch() will return -ERANGE
>> is if the CEPH_OSD_OP_WATCH operation returns that, which it will
>> not.
>>
>> As a result, the whole looping structure and in fact the whole
>> function becomes excessive.  So get rid of rbd_init_watch_dev(),
>> and call rbd_req_sync_watch() directly in the one place it's used.
> 
> Instead of getting rid of this loop, we should make it
> send a compound operation consisting of (CEPH_OSD_OP_ASSERT_VER,
> CEPH_OSD_OP_WATCH). Then we will get -ERANGE if our version
> of the header is out of data, so we don't lose header updates
> before the watch is established.

I don't have compound operations going yet, but now I see why it
may have been written this way.  So I'll drop this patch, and
will make a note to re-visit this spot later.

					-Alex

> Josh
> 
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   18 +-----------------
>>   1 file changed, 1 insertion(+), 17 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 94d0745..71e3f3b 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -2191,22 +2191,6 @@ static void rbd_bus_del_dev(struct rbd_device
>> *rbd_dev)
>>       device_unregister(&rbd_dev->dev);
>>   }
>>
>> -static int rbd_init_watch_dev(struct rbd_device *rbd_dev)
>> -{
>> -    int ret, rc;
>> -
>> -    do {
>> -        ret = rbd_req_sync_watch(rbd_dev);
>> -        if (ret == -ERANGE) {
>> -            rc = rbd_refresh_header(rbd_dev, NULL);
>> -            if (rc < 0)
>> -                return rc;
>> -        }
>> -    } while (ret == -ERANGE);
>> -
>> -    return ret;
>> -}
>> -
>>   static atomic64_t rbd_id_max = ATOMIC64_INIT(0);
>>
>>   /*
>> @@ -2510,7 +2494,7 @@ static ssize_t rbd_add(struct bus_type *bus,
>>       if (rc)
>>           goto err_out_bus;
>>
>> -    rc = rbd_init_watch_dev(rbd_dev);
>> +    rc = rbd_req_sync_watch(rbd_dev);
>>       if (rc)
>>           goto err_out_bus;
>>
> 
> 
>