Re: [PATCH 2/2] rbd: prevent open for image being removed
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 > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] rbd: prevent open for image being removed
Enums should be capitalized according to Documentation/CodingStyle. Other than that, looks good. Reviewed-by: Josh Durgin 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 Signed-off-by: Alex Elder --- 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 { charname[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; -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] rbd: prevent open for image being removed
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 Signed-off-by: Alex Elder --- 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 { charname[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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] rbd: prevent open for image being removed
An open request for a mapped rbd image can arrive while removal of that mapping is underway. The control mutex and an open count is protect a mapped device that's in use from being removed. But it is possible for the removal of the mapping to reach the point of no return *after* a racing open has concluded it is OK to proceed. The result of this is not good. Define and use a flag to indicate a mapping is getting removed to avoid this problem. This addresses http://tracker.newdream.net/issues/3427 Signed-off-by: Alex Elder --- drivers/block/rbd.c | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 9eb1631..760f7f7 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -264,6 +264,7 @@ struct rbd_device { 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 */ @@ -351,17 +352,22 @@ 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; + int ret = 0; if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only) return -EROFS; 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++; + 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); - return 0; + return ret; } static int rbd_release(struct gendisk *disk, fmode_t mode) @@ -3796,6 +3802,7 @@ static ssize_t rbd_remove(struct bus_type *bus, ret = -EBUSY; goto done; } + set_bit(rbd_dev_flag_removing, &rbd_dev->flags); rbd_remove_all_snaps(rbd_dev); rbd_bus_del_dev(rbd_dev); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html