[PATCH 0/2] rbd: manage racing opens/removes

[PATCH 0/2] rbd: manage racing opens/removes

[Alex Elder]

A recent change to rbd prevented rbd devices from being unmapped
when they were in use.  However that change did not address a
different, but related problem.  It is possible for an open (the
one that would bump the open count from 0 to 1) to begin after
a request to remove the rbd device has decided it can proceed.

To fix this, define a new "removing" flag to prevent opens from
proceeding once ermoval of a device has begun.  The first patch
in this series defines a new flags field, and uses it for this
as well as the "exists" flag for snapshot mappings.

					-Alex

[PATCH 1/2] rbd: define flags field, use it for exists flag
[PATCH 2/2] rbd: prevent open for image being removed

[PATCH 1/2] rbd: define flags field, use it for exists flag

[Alex Elder]

Define a new rbd device flags field, manipulated using bit
operations.  Replace the use of the current "exists" flag with a bit
in this new "flags" field.  Add a little commentary about the
"exists" flag, which does not need to be manipulated atomically.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 177ba0c..107df40 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -262,7 +262,7 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */

 	struct rbd_image_header	header;
-	atomic_t		exists;
+	unsigned long		flags;
 	struct rbd_spec		*spec;

 	char			*header_name;
@@ -291,6 +291,12 @@ struct rbd_device {
 	unsigned long		open_count;
 };

+/* Flag bits for rbd_dev->flags */
+
+enum rbd_dev_flags {
+	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
+};
+
 static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */

 static LIST_HEAD(rbd_dev_list);    /* devices */
@@ -790,7 +796,8 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev)
 			goto done;
 		rbd_dev->mapping.read_only = true;
 	}
-	atomic_set(&rbd_dev->exists, 1);
+	set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
+
 done:
 	return ret;
 }
@@ -1886,9 +1893,14 @@ static void rbd_request_fn(struct request_queue *q)
 			rbd_assert(rbd_dev->spec->snap_id == CEPH_NOSNAP);
 		}

-		/* Quit early if the snapshot has disappeared */
-
-		if (!atomic_read(&rbd_dev->exists)) {
+		/*
+		 * Quit early if the mapped snapshot no longer
+		 * exists.  It's still possible the snapshot will
+		 * have disappeared by the time our request arrives
+		 * at the osd, but there's no sense in sending it if
+		 * we already know.
+		 */
+		if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
 			dout("request for non-existent snapshot");
 			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
 			result = -ENXIO;
@@ -2578,7 +2590,7 @@ struct rbd_device *rbd_dev_create(struct
rbd_client *rbdc,
 		return NULL;

 	spin_lock_init(&rbd_dev->lock);
-	atomic_set(&rbd_dev->exists, 0);
+	rbd_dev->flags = 0;
 	INIT_LIST_HEAD(&rbd_dev->node);
 	INIT_LIST_HEAD(&rbd_dev->snaps);
 	init_rwsem(&rbd_dev->header_rwsem);
@@ -3207,10 +3219,17 @@ static int rbd_dev_snaps_update(struct
rbd_device *rbd_dev)
 		if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
 			struct list_head *next = links->next;

-			/* Existing snapshot not in the new snap context */
-
+			/*
+			 * A previously-existing snapshot is not in
+			 * the new snap context.
+			 *
+			 * If the now missing snapshot is the one the
+			 * image is mapped to, clear its exists flag
+			 * so we can avoid sending any more requests
+			 * to it.
+			 */
 			if (rbd_dev->spec->snap_id == snap->id)
-				atomic_set(&rbd_dev->exists, 0);
+				clear_bit(rbd_dev_flag_exists, &rbd_dev->flags);
 			rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
 				rbd_dev->spec->snap_id == snap->id ?
-- 
1.7.9.5

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 01/28/2013 02:09 PM, Alex Elder wrote:
> Define a new rbd device flags field, manipulated using bit
> operations.  Replace the use of the current "exists" flag with a bit
> in this new "flags" field.  Add a little commentary about the
> "exists" flag, which does not need to be manipulated atomically.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   37 ++++++++++++++++++++++++++++---------
>   1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 177ba0c..107df40 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -262,7 +262,7 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> -	atomic_t		exists;
> +	unsigned long		flags;
>   	struct rbd_spec		*spec;
>
>   	char			*header_name;
> @@ -291,6 +291,12 @@ struct rbd_device {
>   	unsigned long		open_count;
>   };
>
> +/* Flag bits for rbd_dev->flags */
> +
> +enum rbd_dev_flags {
> +	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
> +};
> +
>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
>
>   static LIST_HEAD(rbd_dev_list);    /* devices */
> @@ -790,7 +796,8 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev)
>   			goto done;
>   		rbd_dev->mapping.read_only = true;
>   	}
> -	atomic_set(&rbd_dev->exists, 1);
> +	set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
> +
>   done:
>   	return ret;
>   }
> @@ -1886,9 +1893,14 @@ static void rbd_request_fn(struct request_queue *q)
>   			rbd_assert(rbd_dev->spec->snap_id == CEPH_NOSNAP);
>   		}
>
> -		/* Quit early if the snapshot has disappeared */
> -
> -		if (!atomic_read(&rbd_dev->exists)) {
> +		/*
> +		 * Quit early if the mapped snapshot no longer
> +		 * exists.  It's still possible the snapshot will
> +		 * have disappeared by the time our request arrives
> +		 * at the osd, but there's no sense in sending it if
> +		 * we already know.
> +		 */
> +		if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>   			dout("request for non-existent snapshot");
>   			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>   			result = -ENXIO;
> @@ -2578,7 +2590,7 @@ struct rbd_device *rbd_dev_create(struct
> rbd_client *rbdc,
>   		return NULL;
>
>   	spin_lock_init(&rbd_dev->lock);
> -	atomic_set(&rbd_dev->exists, 0);
> +	rbd_dev->flags = 0;
>   	INIT_LIST_HEAD(&rbd_dev->node);
>   	INIT_LIST_HEAD(&rbd_dev->snaps);
>   	init_rwsem(&rbd_dev->header_rwsem);
> @@ -3207,10 +3219,17 @@ static int rbd_dev_snaps_update(struct
> rbd_device *rbd_dev)
>   		if (snap_id == CEPH_NOSNAP || (snap && snap->id > snap_id)) {
>   			struct list_head *next = links->next;
>
> -			/* Existing snapshot not in the new snap context */
> -
> +			/*
> +			 * A previously-existing snapshot is not in
> +			 * the new snap context.
> +			 *
> +			 * If the now missing snapshot is the one the
> +			 * image is mapped to, clear its exists flag
> +			 * so we can avoid sending any more requests
> +			 * to it.
> +			 */
>   			if (rbd_dev->spec->snap_id == snap->id)
> -				atomic_set(&rbd_dev->exists, 0);
> +				clear_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>   			rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
>   				rbd_dev->spec->snap_id == snap->id ?
>

[PATCH 2/2] rbd: prevent open for image being removed

[Alex Elder]

An open request for a mapped rbd image can arrive while removal of
that mapping is underway.  We need to prevent such an open request
from succeeding.  (It appears that Maciej Galkiewicz ran into this
problem.)

Define and use a "removing" flag to indicate a mapping is getting
removed.  Set it in the remove path after verifying nothing holds
the device open.  And check it in the open path before allowing the
open to proceed.  Acquire the rbd device's lock around each of these
spots to avoid any races accessing the flags and open_count fields.

This addresses:
    http://tracker.newdream.net/issues/3427

Reported-by:  Maciej Galkiewicz <maciejgalkiewicz@ragnarson.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 107df40..03b15b8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -259,10 +259,10 @@ struct rbd_device {

 	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */

-	spinlock_t		lock;		/* queue lock */
+	spinlock_t		lock;		/* queue, flags, open_count */

 	struct rbd_image_header	header;
-	unsigned long		flags;
+	unsigned long		flags;		/* possibly lock protected */
 	struct rbd_spec		*spec;

 	char			*header_name;
@@ -288,13 +288,20 @@ struct rbd_device {

 	/* sysfs related */
 	struct device		dev;
-	unsigned long		open_count;
+	unsigned long		open_count;	/* protected by lock */
 };

-/* Flag bits for rbd_dev->flags */
+/*
+ * Flag bits for rbd_dev->flags.  If atomicity is required,
+ * rbd_dev->lock is used to protect access.
+ *
+ * Currently, only the "removing" flag (which is coupled with the
+ * "open_count" field) requires atomic access.
+ */

 enum rbd_dev_flags {
 	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
+	rbd_dev_flag_removing,	/* this mapping is being removed */
 };

 static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
@@ -383,14 +390,23 @@ static int rbd_dev_v2_refresh(struct rbd_device
*rbd_dev, u64 *hver);
 static int rbd_open(struct block_device *bdev, fmode_t mode)
 {
 	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
+	bool removing = false;

 	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
 		return -EROFS;

+	spin_lock(&rbd_dev->lock);
+	if (test_bit(rbd_dev_flag_removing, &rbd_dev->flags))
+		removing = true;
+	else
+		rbd_dev->open_count++;
+	spin_unlock(&rbd_dev->lock);
+	if (removing)
+		return -ENOENT;
+
 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
 	(void) get_device(&rbd_dev->dev);
 	set_device_ro(bdev, rbd_dev->mapping.read_only);
-	rbd_dev->open_count++;
 	mutex_unlock(&ctl_mutex);

 	return 0;
@@ -399,10 +415,14 @@ static int rbd_open(struct block_device *bdev,
fmode_t mode)
 static int rbd_release(struct gendisk *disk, fmode_t mode)
 {
 	struct rbd_device *rbd_dev = disk->private_data;
+	unsigned long open_count_before;
+
+	spin_lock(&rbd_dev->lock);
+	open_count_before = rbd_dev->open_count--;
+	spin_unlock(&rbd_dev->lock);
+	rbd_assert(open_count_before > 0);

 	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
-	rbd_assert(rbd_dev->open_count > 0);
-	rbd_dev->open_count--;
 	put_device(&rbd_dev->dev);
 	mutex_unlock(&ctl_mutex);

@@ -4135,10 +4155,14 @@ static ssize_t rbd_remove(struct bus_type *bus,
 		goto done;
 	}

-	if (rbd_dev->open_count) {
+	spin_lock(&rbd_dev->lock);
+	if (rbd_dev->open_count)
 		ret = -EBUSY;
+	else
+		set_bit(rbd_dev_flag_removing, &rbd_dev->flags);
+	spin_unlock(&rbd_dev->lock);
+	if (ret < 0)
 		goto done;
-	}

 	while (rbd_dev->parent_spec) {
 		struct rbd_device *first = rbd_dev;
-- 
1.7.9.5

Re: [PATCH 2/2] rbd: prevent open for image being removed

[Josh Durgin]

Enums should be capitalized according to Documentation/CodingStyle.

Other than that, looks good.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 01/28/2013 02:09 PM, Alex Elder wrote:
> An open request for a mapped rbd image can arrive while removal of
> that mapping is underway.  We need to prevent such an open request
> from succeeding.  (It appears that Maciej Galkiewicz ran into this
> problem.)
>
> Define and use a "removing" flag to indicate a mapping is getting
> removed.  Set it in the remove path after verifying nothing holds
> the device open.  And check it in the open path before allowing the
> open to proceed.  Acquire the rbd device's lock around each of these
> spots to avoid any races accessing the flags and open_count fields.
>
> This addresses:
>      http://tracker.newdream.net/issues/3427
>
> Reported-by:  Maciej Galkiewicz <maciejgalkiewicz@ragnarson.com>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   42 +++++++++++++++++++++++++++++++++---------
>   1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 107df40..03b15b8 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -259,10 +259,10 @@ struct rbd_device {
>
>   	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */
>
> -	spinlock_t		lock;		/* queue lock */
> +	spinlock_t		lock;		/* queue, flags, open_count */
>
>   	struct rbd_image_header	header;
> -	unsigned long		flags;
> +	unsigned long		flags;		/* possibly lock protected */
>   	struct rbd_spec		*spec;
>
>   	char			*header_name;
> @@ -288,13 +288,20 @@ struct rbd_device {
>
>   	/* sysfs related */
>   	struct device		dev;
> -	unsigned long		open_count;
> +	unsigned long		open_count;	/* protected by lock */
>   };
>
> -/* Flag bits for rbd_dev->flags */
> +/*
> + * Flag bits for rbd_dev->flags.  If atomicity is required,
> + * rbd_dev->lock is used to protect access.
> + *
> + * Currently, only the "removing" flag (which is coupled with the
> + * "open_count" field) requires atomic access.
> + */
>
>   enum rbd_dev_flags {
>   	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
> +	rbd_dev_flag_removing,	/* this mapping is being removed */
>   };
>
>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
> @@ -383,14 +390,23 @@ static int rbd_dev_v2_refresh(struct rbd_device
> *rbd_dev, u64 *hver);
>   static int rbd_open(struct block_device *bdev, fmode_t mode)
>   {
>   	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
> +	bool removing = false;
>
>   	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
>   		return -EROFS;
>
> +	spin_lock(&rbd_dev->lock);
> +	if (test_bit(rbd_dev_flag_removing, &rbd_dev->flags))
> +		removing = true;
> +	else
> +		rbd_dev->open_count++;
> +	spin_unlock(&rbd_dev->lock);
> +	if (removing)
> +		return -ENOENT;
> +
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
>   	(void) get_device(&rbd_dev->dev);
>   	set_device_ro(bdev, rbd_dev->mapping.read_only);
> -	rbd_dev->open_count++;
>   	mutex_unlock(&ctl_mutex);
>
>   	return 0;
> @@ -399,10 +415,14 @@ static int rbd_open(struct block_device *bdev,
> fmode_t mode)
>   static int rbd_release(struct gendisk *disk, fmode_t mode)
>   {
>   	struct rbd_device *rbd_dev = disk->private_data;
> +	unsigned long open_count_before;
> +
> +	spin_lock(&rbd_dev->lock);
> +	open_count_before = rbd_dev->open_count--;
> +	spin_unlock(&rbd_dev->lock);
> +	rbd_assert(open_count_before > 0);
>
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	rbd_assert(rbd_dev->open_count > 0);
> -	rbd_dev->open_count--;
>   	put_device(&rbd_dev->dev);
>   	mutex_unlock(&ctl_mutex);
>
> @@ -4135,10 +4155,14 @@ static ssize_t rbd_remove(struct bus_type *bus,
>   		goto done;
>   	}
>
> -	if (rbd_dev->open_count) {
> +	spin_lock(&rbd_dev->lock);
> +	if (rbd_dev->open_count)
>   		ret = -EBUSY;
> +	else
> +		set_bit(rbd_dev_flag_removing, &rbd_dev->flags);
> +	spin_unlock(&rbd_dev->lock);
> +	if (ret < 0)
>   		goto done;
> -	}
>
>   	while (rbd_dev->parent_spec) {
>   		struct rbd_device *first = rbd_dev;
>

Re: [PATCH 2/2] rbd: prevent open for image being removed

[Alex Elder]

On 01/30/2013 01:52 PM, Josh Durgin wrote:
> Enums should be capitalized according to Documentation/CodingStyle.

I already updated that in my own copy after last time...
Thanks.

					-Alex

> Other than that, looks good.
> Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
> 

[PATCH 0/2] rbd: prevent open of image being unmapped

[Alex Elder]

This series protects an open of a mapped rbd image from succeeding
once an unmap of that image is underway.

Note:  Once committed these should be back-ported.

					-Alex

[PATCH 1/2] rbd: define flags field, use it for exists flag
[PATCH 2/2] rbd: prevent open for image being removed

[PATCH 1/2] rbd: define flags field, use it for exists flag

[Alex Elder]

Define a new rbd device flags field, manipulated using atomic bit
operations.  Replace the use of the current "exists" flag with a
bit in this new "flags" field.

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

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 02002b1..9eb1631 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -232,7 +232,7 @@ struct rbd_device {
 	spinlock_t		lock;		/* queue lock */

 	struct rbd_image_header	header;
-	atomic_t		exists;
+	unsigned long		flags;
 	struct rbd_spec		*spec;

 	char			*header_name;
@@ -260,6 +260,12 @@ struct rbd_device {
 	unsigned long		open_count;
 };

+/* Flag bits for rbd_dev->flags */
+
+enum rbd_dev_flags {
+	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
+};
+
 static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */

 static LIST_HEAD(rbd_dev_list);    /* devices */
@@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
*rbd_dev)
 			goto done;
 		rbd_dev->mapping.read_only = true;
 	}
-	atomic_set(&rbd_dev->exists, 1);
+	set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
+
 done:
 	return ret;
 }
@@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
 			snapc = ceph_get_snap_context(rbd_dev->header.snapc);
 			up_read(&rbd_dev->header_rwsem);
 			rbd_assert(snapc != NULL);
-		} else if (!atomic_read(&rbd_dev->exists)) {
+		} else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
 			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
 			dout("request for non-existent snapshot");
 			result = -ENXIO;
@@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
rbd_client *rbdc,
 		return NULL;

 	spin_lock_init(&rbd_dev->lock);
-	atomic_set(&rbd_dev->exists, 0);
+	rbd_dev->flags = 0;
 	INIT_LIST_HEAD(&rbd_dev->node);
 	INIT_LIST_HEAD(&rbd_dev->snaps);
 	init_rwsem(&rbd_dev->header_rwsem);
@@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
*rbd_dev)
 			/* Existing snapshot not in the new snap context */

 			if (rbd_dev->spec->snap_id == snap->id)
-				atomic_set(&rbd_dev->exists, 0);
+				set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
 			rbd_remove_snap_dev(snap);
 			dout("%ssnap id %llu has been removed\n",
 				rbd_dev->spec->snap_id == snap->id ?
-- 
1.7.9.5

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Dan Mick]

I see that set_bit is atomic, but I don't see that test_bit is.  Am I 
missing a subtlety?

On 01/14/2013 10:50 AM, Alex Elder wrote:
> Define a new rbd device flags field, manipulated using atomic bit
> operations.  Replace the use of the current "exists" flag with a
> bit in this new "flags" field.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 02002b1..9eb1631 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -232,7 +232,7 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> -	atomic_t		exists;
> +	unsigned long		flags;
>   	struct rbd_spec		*spec;
>
>   	char			*header_name;
> @@ -260,6 +260,12 @@ struct rbd_device {
>   	unsigned long		open_count;
>   };
>
> +/* Flag bits for rbd_dev->flags */
> +
> +enum rbd_dev_flags {
> +	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
> +};
> +
>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
>
>   static LIST_HEAD(rbd_dev_list);    /* devices */
> @@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev)
>   			goto done;
>   		rbd_dev->mapping.read_only = true;
>   	}
> -	atomic_set(&rbd_dev->exists, 1);
> +	set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
> +
>   done:
>   	return ret;
>   }
> @@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   			snapc = ceph_get_snap_context(rbd_dev->header.snapc);
>   			up_read(&rbd_dev->header_rwsem);
>   			rbd_assert(snapc != NULL);
> -		} else if (!atomic_read(&rbd_dev->exists)) {
> +		} else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>   			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>   			dout("request for non-existent snapshot");
>   			result = -ENXIO;
> @@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
> rbd_client *rbdc,
>   		return NULL;
>
>   	spin_lock_init(&rbd_dev->lock);
> -	atomic_set(&rbd_dev->exists, 0);
> +	rbd_dev->flags = 0;
>   	INIT_LIST_HEAD(&rbd_dev->node);
>   	INIT_LIST_HEAD(&rbd_dev->snaps);
>   	init_rwsem(&rbd_dev->header_rwsem);
> @@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   			/* Existing snapshot not in the new snap context */
>
>   			if (rbd_dev->spec->snap_id == snap->id)
> -				atomic_set(&rbd_dev->exists, 0);
> +				set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>   			rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
>   				rbd_dev->spec->snap_id == snap->id ?
>

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Alex Elder]

On 01/14/2013 02:32 PM, Dan Mick wrote:
> I see that set_bit is atomic, but I don't see that test_bit is.  Am I
> missing a subtlety?

That's an interesting observation.  I'm certain it's safe, but
I needed to research it a bit, and I still haven't verified it
to my satisfaction.

I *think* (but please look over the following and see if you
come to the same conclusion) that this operation doesn't need
to be made atomic, because the implementation of the routines
that implement the "set" operations guarantee their effects are
visible once they are done.

But I'm not sure whether "visible" here means precisely that
another CPU will be forced to go read the updated memory when
it calls test_bit().

http://www.kernel.org/doc/Documentation/atomic_ops.txt
The section of interest can be found by looking for the
sentence I'm talking about:
  Likewise, the atomic bit operation must be visible globally before any
  subsequent memory operation is made visible.

It doesn't come right and explain it though.  Please let me
know what you think.

					-Alex


> On 01/14/2013 10:50 AM, Alex Elder wrote:
>> Define a new rbd device flags field, manipulated using atomic bit
>> operations.  Replace the use of the current "exists" flag with a
>> bit in this new "flags" field.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>   drivers/block/rbd.c |   17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 02002b1..9eb1631 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -232,7 +232,7 @@ struct rbd_device {
>>       spinlock_t        lock;        /* queue lock */
>>
>>       struct rbd_image_header    header;
>> -    atomic_t        exists;
>> +    unsigned long        flags;
>>       struct rbd_spec        *spec;
>>
>>       char            *header_name;
>> @@ -260,6 +260,12 @@ struct rbd_device {
>>       unsigned long        open_count;
>>   };
>>
>> +/* Flag bits for rbd_dev->flags */
>> +
>> +enum rbd_dev_flags {
>> +    rbd_dev_flag_exists,    /* mapped snapshot has not been deleted */
>> +};
>> +
>>   static DEFINE_MUTEX(ctl_mutex);      /* Serialize
>> open/close/setup/teardown */
>>
>>   static LIST_HEAD(rbd_dev_list);    /* devices */
>> @@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
>> *rbd_dev)
>>               goto done;
>>           rbd_dev->mapping.read_only = true;
>>       }
>> -    atomic_set(&rbd_dev->exists, 1);
>> +    set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>> +
>>   done:
>>       return ret;
>>   }
>> @@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
>>               snapc = ceph_get_snap_context(rbd_dev->header.snapc);
>>               up_read(&rbd_dev->header_rwsem);
>>               rbd_assert(snapc != NULL);
>> -        } else if (!atomic_read(&rbd_dev->exists)) {
>> +        } else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>>               rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>>               dout("request for non-existent snapshot");
>>               result = -ENXIO;
>> @@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
>> rbd_client *rbdc,
>>           return NULL;
>>
>>       spin_lock_init(&rbd_dev->lock);
>> -    atomic_set(&rbd_dev->exists, 0);
>> +    rbd_dev->flags = 0;
>>       INIT_LIST_HEAD(&rbd_dev->node);
>>       INIT_LIST_HEAD(&rbd_dev->snaps);
>>       init_rwsem(&rbd_dev->header_rwsem);
>> @@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
>> *rbd_dev)
>>               /* Existing snapshot not in the new snap context */
>>
>>               if (rbd_dev->spec->snap_id == snap->id)
>> -                atomic_set(&rbd_dev->exists, 0);
>> +                set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>>               rbd_remove_snap_dev(snap);
>>               dout("%ssnap id %llu has been removed\n",
>>                   rbd_dev->spec->snap_id == snap->id ?
>>

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Dan Mick]

I think I agree that the claim is that the onus is on the set, and so
I think the proposed code is safe.

On 01/14/2013 01:23 PM, Alex Elder wrote:
> On 01/14/2013 02:32 PM, Dan Mick wrote:
>> I see that set_bit is atomic, but I don't see that test_bit is.  Am I
>> missing a subtlety?
>
> That's an interesting observation.  I'm certain it's safe, but
> I needed to research it a bit, and I still haven't verified it
> to my satisfaction.
>
> I *think* (but please look over the following and see if you
> come to the same conclusion) that this operation doesn't need
> to be made atomic, because the implementation of the routines
> that implement the "set" operations guarantee their effects are
> visible once they are done.
>
> But I'm not sure whether "visible" here means precisely that
> another CPU will be forced to go read the updated memory when
> it calls test_bit().
>
> http://www.kernel.org/doc/Documentation/atomic_ops.txt
> The section of interest can be found by looking for the
> sentence I'm talking about:
>    Likewise, the atomic bit operation must be visible globally before any
>    subsequent memory operation is made visible.
>
> It doesn't come right and explain it though.  Please let me
> know what you think.
>
> 					-Alex
>
>
>> On 01/14/2013 10:50 AM, Alex Elder wrote:
>>> Define a new rbd device flags field, manipulated using atomic bit
>>> operations.  Replace the use of the current "exists" flag with a
>>> bit in this new "flags" field.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   17 ++++++++++++-----
>>>    1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 02002b1..9eb1631 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -232,7 +232,7 @@ struct rbd_device {
>>>        spinlock_t        lock;        /* queue lock */
>>>
>>>        struct rbd_image_header    header;
>>> -    atomic_t        exists;
>>> +    unsigned long        flags;
>>>        struct rbd_spec        *spec;
>>>
>>>        char            *header_name;
>>> @@ -260,6 +260,12 @@ struct rbd_device {
>>>        unsigned long        open_count;
>>>    };
>>>
>>> +/* Flag bits for rbd_dev->flags */
>>> +
>>> +enum rbd_dev_flags {
>>> +    rbd_dev_flag_exists,    /* mapped snapshot has not been deleted */
>>> +};
>>> +
>>>    static DEFINE_MUTEX(ctl_mutex);      /* Serialize
>>> open/close/setup/teardown */
>>>
>>>    static LIST_HEAD(rbd_dev_list);    /* devices */
>>> @@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
>>> *rbd_dev)
>>>                goto done;
>>>            rbd_dev->mapping.read_only = true;
>>>        }
>>> -    atomic_set(&rbd_dev->exists, 1);
>>> +    set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>>> +
>>>    done:
>>>        return ret;
>>>    }
>>> @@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
>>>                snapc = ceph_get_snap_context(rbd_dev->header.snapc);
>>>                up_read(&rbd_dev->header_rwsem);
>>>                rbd_assert(snapc != NULL);
>>> -        } else if (!atomic_read(&rbd_dev->exists)) {
>>> +        } else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>>>                rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>>>                dout("request for non-existent snapshot");
>>>                result = -ENXIO;
>>> @@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
>>> rbd_client *rbdc,
>>>            return NULL;
>>>
>>>        spin_lock_init(&rbd_dev->lock);
>>> -    atomic_set(&rbd_dev->exists, 0);
>>> +    rbd_dev->flags = 0;
>>>        INIT_LIST_HEAD(&rbd_dev->node);
>>>        INIT_LIST_HEAD(&rbd_dev->snaps);
>>>        init_rwsem(&rbd_dev->header_rwsem);
>>> @@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
>>> *rbd_dev)
>>>                /* Existing snapshot not in the new snap context */
>>>
>>>                if (rbd_dev->spec->snap_id == snap->id)
>>> -                atomic_set(&rbd_dev->exists, 0);
>>> +                set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>>>                rbd_remove_snap_dev(snap);
>>>                dout("%ssnap id %llu has been removed\n",
>>>                    rbd_dev->spec->snap_id == snap->id ?
>>>
>

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Josh Durgin]

On 01/14/2013 01:23 PM, Alex Elder wrote:
> On 01/14/2013 02:32 PM, Dan Mick wrote:
>> I see that set_bit is atomic, but I don't see that test_bit is.  Am I
>> missing a subtlety?
>
> That's an interesting observation.  I'm certain it's safe, but
> I needed to research it a bit, and I still haven't verified it
> to my satisfaction.
>
> I *think* (but please look over the following and see if you
> come to the same conclusion) that this operation doesn't need
> to be made atomic, because the implementation of the routines
> that implement the "set" operations guarantee their effects are
> visible once they are done.
>
> But I'm not sure whether "visible" here means precisely that
> another CPU will be forced to go read the updated memory when
> it calls test_bit().
>
> http://www.kernel.org/doc/Documentation/atomic_ops.txt
> The section of interest can be found by looking for the
> sentence I'm talking about:
>    Likewise, the atomic bit operation must be visible globally before any
>    subsequent memory operation is made visible.

I read that differently. I think that only applies to the test_and_set 
style operations mentioned directly above, not set_bit.

Documentation/memory-barriers.txt confirms this interpretation:

     The following operations are potential problems as they do
     _not_ imply memory barriers, but might be used for
     implementing such things as UNLOCK-class operations:

             atomic_set();
             set_bit();
             clear_bit();
             change_bit();

     With these the appropriate explicit memory barrier should be
     used if necessary (smp_mb__before_clear_bit() for instance).

And:

     Memory operations that occur after an UNLOCK operation may appear to
     happen before it completes.

So I think we need a memory barrier before and after set_bit for the
removing flag, but we don't need barriers for the exists flag, since
it's a best-effort value that can't stop already-in-flight requests.

Josh

> It doesn't come right and explain it though.  Please let me
> know what you think.
>
> 					-Alex
>
>
>> On 01/14/2013 10:50 AM, Alex Elder wrote:
>>> Define a new rbd device flags field, manipulated using atomic bit
>>> operations.  Replace the use of the current "exists" flag with a
>>> bit in this new "flags" field.
>>>
>>> Signed-off-by: Alex Elder <elder@inktank.com>
>>> ---
>>>    drivers/block/rbd.c |   17 ++++++++++++-----
>>>    1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>>> index 02002b1..9eb1631 100644
>>> --- a/drivers/block/rbd.c
>>> +++ b/drivers/block/rbd.c
>>> @@ -232,7 +232,7 @@ struct rbd_device {
>>>        spinlock_t        lock;        /* queue lock */
>>>
>>>        struct rbd_image_header    header;
>>> -    atomic_t        exists;
>>> +    unsigned long        flags;
>>>        struct rbd_spec        *spec;
>>>
>>>        char            *header_name;
>>> @@ -260,6 +260,12 @@ struct rbd_device {
>>>        unsigned long        open_count;
>>>    };
>>>
>>> +/* Flag bits for rbd_dev->flags */
>>> +
>>> +enum rbd_dev_flags {
>>> +    rbd_dev_flag_exists,    /* mapped snapshot has not been deleted */
>>> +};
>>> +
>>>    static DEFINE_MUTEX(ctl_mutex);      /* Serialize
>>> open/close/setup/teardown */
>>>
>>>    static LIST_HEAD(rbd_dev_list);    /* devices */
>>> @@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
>>> *rbd_dev)
>>>                goto done;
>>>            rbd_dev->mapping.read_only = true;
>>>        }
>>> -    atomic_set(&rbd_dev->exists, 1);
>>> +    set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>>> +
>>>    done:
>>>        return ret;
>>>    }
>>> @@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
>>>                snapc = ceph_get_snap_context(rbd_dev->header.snapc);
>>>                up_read(&rbd_dev->header_rwsem);
>>>                rbd_assert(snapc != NULL);
>>> -        } else if (!atomic_read(&rbd_dev->exists)) {
>>> +        } else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>>>                rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>>>                dout("request for non-existent snapshot");
>>>                result = -ENXIO;
>>> @@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
>>> rbd_client *rbdc,
>>>            return NULL;
>>>
>>>        spin_lock_init(&rbd_dev->lock);
>>> -    atomic_set(&rbd_dev->exists, 0);
>>> +    rbd_dev->flags = 0;
>>>        INIT_LIST_HEAD(&rbd_dev->node);
>>>        INIT_LIST_HEAD(&rbd_dev->snaps);
>>>        init_rwsem(&rbd_dev->header_rwsem);
>>> @@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
>>> *rbd_dev)
>>>                /* Existing snapshot not in the new snap context */
>>>
>>>                if (rbd_dev->spec->snap_id == snap->id)
>>> -                atomic_set(&rbd_dev->exists, 0);
>>> +                set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>>>                rbd_remove_snap_dev(snap);
>>>                dout("%ssnap id %llu has been removed\n",
>>>                    rbd_dev->spec->snap_id == snap->id ?
>>>

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Alex Elder]

On 01/15/2013 07:08 PM, Josh Durgin wrote:
> On 01/14/2013 01:23 PM, Alex Elder wrote:
>> On 01/14/2013 02:32 PM, Dan Mick wrote:
>>> I see that set_bit is atomic, but I don't see that test_bit is.  Am I
>>> missing a subtlety?
>>
>> That's an interesting observation.  I'm certain it's safe, but
>> I needed to research it a bit, and I still haven't verified it
>> to my satisfaction.
>>
>> I *think* (but please look over the following and see if you
>> come to the same conclusion) that this operation doesn't need
>> to be made atomic, because the implementation of the routines
>> that implement the "set" operations guarantee their effects are
>> visible once they are done.
>>
>> But I'm not sure whether "visible" here means precisely that
>> another CPU will be forced to go read the updated memory when
>> it calls test_bit().
>>
>> http://www.kernel.org/doc/Documentation/atomic_ops.txt
>> The section of interest can be found by looking for the
>> sentence I'm talking about:
>>    Likewise, the atomic bit operation must be visible globally before any
>>    subsequent memory operation is made visible.
> 
> I read that differently. I think that only applies to the test_and_set
> style operations mentioned directly above, not set_bit.
> 
> Documentation/memory-barriers.txt confirms this interpretation:
> 
>     The following operations are potential problems as they do
>     _not_ imply memory barriers, but might be used for
>     implementing such things as UNLOCK-class operations:
> 
>             atomic_set();
>             set_bit();
>             clear_bit();
>             change_bit();
> 
>     With these the appropriate explicit memory barrier should be
>     used if necessary (smp_mb__before_clear_bit() for instance).
> 
> And:
> 
>     Memory operations that occur after an UNLOCK operation may appear to
>     happen before it completes.
> 
> So I think we need a memory barrier before and after set_bit for the
> removing flag, but we don't need barriers for the exists flag, since
> it's a best-effort value that can't stop already-in-flight requests.

You know, I agree with your analysis but now I'm not sure
even that's enough.

Here's the code in question (from the other patch):

Test side:
        mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
        if (!test_bit(rbd_dev_flag_removing, &rbd_dev->flags)) {
                (void) get_device(&rbd_dev->dev);
                set_device_ro(bdev, rbd_dev->mapping.read_only);
                rbd_dev->open_count++;
        } else {
                ret = -ENOENT;
        }
        mutex_unlock(&ctl_mutex);


Set side:
        if (rbd_dev->open_count) {
                ret = -EBUSY;
                goto done;
        }
        set_bit(rbd_dev_flag_removing, &rbd_dev->flags);

And here's the scenario I'm thinking about.  Initially,
suppose rbd_dev->open_count is 0 and the removing flag
is not set.

OPENING THREAD                  UNMAPPING THREAD
--------------                  ----------------
                                if (rbd_dev->open_count) {
                                    /* not taken, it's zero */
                                    ret = -EBUSY;
                                    goto done;
                                }
if (!test_bit(removing)) {
    /* not set yet! */          /* barrier won't help here */
                                set_bit(removing);
                                /* clean stuff up */
    rbd_dev->open_count++;      /* == kablooie == */
} else {
    ret = -ENOENT;
}

So I think we need a spinlock, or some other thing.

In any case, I'm not going to commit this change until
we've had a chance to talk about it a little more.

					-Alex

Re: [PATCH 1/2] rbd: define flags field, use it for exists flag

[Dan Mick]

Reviewed-by: Dan Mick <dan.mick@inktank.com>

On 01/14/2013 10:50 AM, Alex Elder wrote:
> Define a new rbd device flags field, manipulated using atomic bit
> operations.  Replace the use of the current "exists" flag with a
> bit in this new "flags" field.
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 02002b1..9eb1631 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -232,7 +232,7 @@ struct rbd_device {
>   	spinlock_t		lock;		/* queue lock */
>
>   	struct rbd_image_header	header;
> -	atomic_t		exists;
> +	unsigned long		flags;
>   	struct rbd_spec		*spec;
>
>   	char			*header_name;
> @@ -260,6 +260,12 @@ struct rbd_device {
>   	unsigned long		open_count;
>   };
>
> +/* Flag bits for rbd_dev->flags */
> +
> +enum rbd_dev_flags {
> +	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
> +};
> +
>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
>
>   static LIST_HEAD(rbd_dev_list);    /* devices */
> @@ -756,7 +762,8 @@ static int rbd_dev_set_mapping(struct rbd_device
> *rbd_dev)
>   			goto done;
>   		rbd_dev->mapping.read_only = true;
>   	}
> -	atomic_set(&rbd_dev->exists, 1);
> +	set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
> +
>   done:
>   	return ret;
>   }
> @@ -1654,7 +1661,7 @@ static void rbd_rq_fn(struct request_queue *q)
>   			snapc = ceph_get_snap_context(rbd_dev->header.snapc);
>   			up_read(&rbd_dev->header_rwsem);
>   			rbd_assert(snapc != NULL);
> -		} else if (!atomic_read(&rbd_dev->exists)) {
> +		} else if (!test_bit(rbd_dev_flag_exists, &rbd_dev->flags)) {
>   			rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP);
>   			dout("request for non-existent snapshot");
>   			result = -ENXIO;
> @@ -2270,7 +2277,7 @@ struct rbd_device *rbd_dev_create(struct
> rbd_client *rbdc,
>   		return NULL;
>
>   	spin_lock_init(&rbd_dev->lock);
> -	atomic_set(&rbd_dev->exists, 0);
> +	rbd_dev->flags = 0;
>   	INIT_LIST_HEAD(&rbd_dev->node);
>   	INIT_LIST_HEAD(&rbd_dev->snaps);
>   	init_rwsem(&rbd_dev->header_rwsem);
> @@ -2902,7 +2909,7 @@ static int rbd_dev_snaps_update(struct rbd_device
> *rbd_dev)
>   			/* Existing snapshot not in the new snap context */
>
>   			if (rbd_dev->spec->snap_id == snap->id)
> -				atomic_set(&rbd_dev->exists, 0);
> +				set_bit(rbd_dev_flag_exists, &rbd_dev->flags);
>   			rbd_remove_snap_dev(snap);
>   			dout("%ssnap id %llu has been removed\n",
>   				rbd_dev->spec->snap_id == snap->id ?
>