| Message ID | 1312295808-4323-3-git-send-email-ejt@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Alasdair Kergon |
| Headers | show |
I think this is not correct. The problem here is that the block may have been recycled and the newly created block may have the same block number as the old block. If b->where != block, we know that the block was recycled. If b->where == block, the block may have been recycled or not and we don't know. I think the correct solution could be: make write_lock_pending a boolean variable, not a counter. Set write_lock_pending inside __wait_block when we are about to wait (the block may have been recycled each time we waited, so we need to set it each time we are going to wait) Clear write_lock_pending when __wait_unlocked exits. If we make it a boolean variable, double clearing makes no harm. Mikulas On Tue, 2 Aug 2011, Joe Thornber wrote: > --- > drivers/md/persistent-data/dm-block-manager.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/drivers/md/persistent-data/dm-block-manager.c b/drivers/md/persistent-data/dm-block-manager.c > index b68be88..d27ab6e 100644 > --- a/drivers/md/persistent-data/dm-block-manager.c > +++ b/drivers/md/persistent-data/dm-block-manager.c > @@ -756,9 +756,15 @@ retry: > > b->write_lock_pending++; > __wait_unlocked(b, &flags); > - b->write_lock_pending--; > if (b->where != block) > + /* > + * Recycled blocks have their > + * write_lock_pending count reset > + * to zero, so no need to undo the > + * above increment. > + */ > goto retry; > + b->write_lock_pending--; > } > break; > } > -- > 1.7.4.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Aug 03, 2011 at 10:50:33AM -0400, Mikulas Patocka wrote: > I think this is not correct. I had a similar thought last night, however my concern was the previous 'read' patch that you've acked. I'll go back and look at these today. - Joe > > The problem here is that the block may have been recycled and the newly > created block may have the same block number as the old block. > > If b->where != block, we know that the block was recycled. > If b->where == block, the block may have been recycled or not and we > don't know. > > > I think the correct solution could be: make write_lock_pending a boolean > variable, not a counter. > > Set write_lock_pending inside __wait_block when we are about to wait (the > block may have been recycled each time we waited, so we need to set it > each time we are going to wait) > Clear write_lock_pending when __wait_unlocked exits. > > If we make it a boolean variable, double clearing makes no harm. > > Mikulas > > On Tue, 2 Aug 2011, Joe Thornber wrote: > > > --- > > drivers/md/persistent-data/dm-block-manager.c | 8 +++++++- > > 1 files changed, 7 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/md/persistent-data/dm-block-manager.c b/drivers/md/persistent-data/dm-block-manager.c > > index b68be88..d27ab6e 100644 > > --- a/drivers/md/persistent-data/dm-block-manager.c > > +++ b/drivers/md/persistent-data/dm-block-manager.c > > @@ -756,9 +756,15 @@ retry: > > > > b->write_lock_pending++; > > __wait_unlocked(b, &flags); > > - b->write_lock_pending--; > > if (b->where != block) > > + /* > > + * Recycled blocks have their > > + * write_lock_pending count reset > > + * to zero, so no need to undo the > > + * above increment. > > + */ > > goto retry; > > + b->write_lock_pending--; > > } > > break; > > } > > -- > > 1.7.4.1 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/persistent-data/dm-block-manager.c b/drivers/md/persistent-data/dm-block-manager.c index b68be88..d27ab6e 100644 --- a/drivers/md/persistent-data/dm-block-manager.c +++ b/drivers/md/persistent-data/dm-block-manager.c @@ -756,9 +756,15 @@ retry: b->write_lock_pending++; __wait_unlocked(b, &flags); - b->write_lock_pending--; if (b->where != block) + /* + * Recycled blocks have their + * write_lock_pending count reset + * to zero, so no need to undo the + * above increment. + */ goto retry; + b->write_lock_pending--; } break; }