* [PATCH 0/2] rbd: manage racing opens/removes @ 2013-01-28 22:08 Alex Elder 2013-01-28 22:09 ` [PATCH 1/2] rbd: define flags field, use it for exists flag Alex Elder 2013-01-28 22:09 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder 0 siblings, 2 replies; 7+ messages in thread From: Alex Elder @ 2013-01-28 22:08 UTC (permalink / raw) To: ceph-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] rbd: define flags field, use it for exists flag 2013-01-28 22:08 [PATCH 0/2] rbd: manage racing opens/removes Alex Elder @ 2013-01-28 22:09 ` Alex Elder 2013-01-30 19:45 ` Josh Durgin 2013-01-28 22:09 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder 1 sibling, 1 reply; 7+ messages in thread From: Alex Elder @ 2013-01-28 22:09 UTC (permalink / raw) To: ceph-devel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] rbd: define flags field, use it for exists flag 2013-01-28 22:09 ` [PATCH 1/2] rbd: define flags field, use it for exists flag Alex Elder @ 2013-01-30 19:45 ` Josh Durgin 0 siblings, 0 replies; 7+ messages in thread From: Josh Durgin @ 2013-01-30 19:45 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel 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 ? > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] rbd: prevent open for image being removed 2013-01-28 22:08 [PATCH 0/2] rbd: manage racing opens/removes Alex Elder 2013-01-28 22:09 ` [PATCH 1/2] rbd: define flags field, use it for exists flag Alex Elder @ 2013-01-28 22:09 ` Alex Elder 2013-01-30 19:52 ` Josh Durgin 1 sibling, 1 reply; 7+ messages in thread From: Alex Elder @ 2013-01-28 22:09 UTC (permalink / raw) To: ceph-devel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] rbd: prevent open for image being removed 2013-01-28 22:09 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder @ 2013-01-30 19:52 ` Josh Durgin 2013-01-30 21:25 ` Alex Elder 0 siblings, 1 reply; 7+ messages in thread From: Josh Durgin @ 2013-01-30 19:52 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] rbd: prevent open for image being removed 2013-01-30 19:52 ` Josh Durgin @ 2013-01-30 21:25 ` Alex Elder 0 siblings, 0 replies; 7+ messages in thread From: Alex Elder @ 2013-01-30 21:25 UTC (permalink / raw) To: Josh Durgin; +Cc: ceph-devel 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> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/2] rbd: prevent open of image being unmapped @ 2013-01-14 18:50 Alex Elder 2013-01-14 18:51 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder 0 siblings, 1 reply; 7+ messages in thread From: Alex Elder @ 2013-01-14 18:50 UTC (permalink / raw) To: ceph-devel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] rbd: prevent open for image being removed 2013-01-14 18:50 [PATCH 0/2] rbd: prevent open of image being unmapped Alex Elder @ 2013-01-14 18:51 ` Alex Elder 0 siblings, 0 replies; 7+ messages in thread From: Alex Elder @ 2013-01-14 18:51 UTC (permalink / raw) To: ceph-devel@vger.kernel.org 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 <elder@inktank.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-01-30 21:25 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-28 22:08 [PATCH 0/2] rbd: manage racing opens/removes Alex Elder 2013-01-28 22:09 ` [PATCH 1/2] rbd: define flags field, use it for exists flag Alex Elder 2013-01-30 19:45 ` Josh Durgin 2013-01-28 22:09 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder 2013-01-30 19:52 ` Josh Durgin 2013-01-30 21:25 ` Alex Elder -- strict thread matches above, loose matches on Subject: below -- 2013-01-14 18:50 [PATCH 0/2] rbd: prevent open of image being unmapped Alex Elder 2013-01-14 18:51 ` [PATCH 2/2] rbd: prevent open for image being removed Alex Elder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.