block: make blkdev_get/put() handle exclusive access
Over time, block layer has accumulated a set of APIs dealing with bdev
open, close, claim and release.
* blkdev_get/put() are the primary open and close functions.
* bd_claim/release() deal with exclusive open.
* open/close_bdev_exclusive() are combination of open and claim and
the other way around, respectively.
* bd_link/unlink_disk_holder() to create and remove holder/slave
symlinks.
* open_by_devnum() wraps bdget() + blkdev_get().
The interface is a bit confusing and the decoupling of open and claim
makes it impossible to properly guarantee exclusive access as
in-kernel open + claim sequence can disturb the existing exclusive
open even before the block layer knows the current open if for another
exclusive access. Reorganize the interface such that,
* blkdev_get() is extended to include exclusive access management.
@holder argument is added and, if is @FMODE_EXCL specified, it will
gain exclusive access atomically w.r.t. other exclusive accesses.
* blkdev_put() is similarly extended. It now takes @mode argument and
if @FMODE_EXCL is set, it releases an exclusive access. Also, when
the last exclusive claim is released, the holder/slave symlinks are
removed automatically.
* bd_claim/release() and close_bdev_exclusive() are no longer
necessary and either made static or removed.
* bd_link_disk_holder() remains the same but bd_unlink_disk_holder()
is no longer necessary and removed.
* open_bdev_exclusive() becomes a simple wrapper around lookup_bdev()
and blkdev_get(). It also has an unexpected extra bdev_read_only()
test which probably should be moved into blkdev_get().
* open_by_devnum() is modified to take @holder argument and pass it to
blkdev_get().
Most of bdev open/close operations are unified into blkdev_get/put()
and most exclusive accesses are tested atomically at the open time (as
it should). This cleans up code and removes some, both valid and
invalid, but unnecessary all the same, corner cases.
open_bdev_exclusive() and open_by_devnum() can use further cleanup -
rename to blkdev_get_by_path() and blkdev_get_by_devt() and drop
special features. Well, let's leave them for another day.
Most conversions are straight-forward. drbd conversion is a bit more
involved as there was some reordering, but the logic should stay the
same.
@@ -950,28 +944,7 @@ static int drbd_nl_disk_conf(struct drbd_conf *mdev, struct drbd_nl_cfg_req *nlp
offsetof(struct bm_extent, lce));
if (!resync_lru) {
retcode = ERR_NOMEM;
- goto release_bdev_fail;
- }
-
- /* meta_dev_idx >= 0: external fixed size,
- * possibly multiple drbd sharing one meta device.
- * TODO in that case, paranoia check that [md_bdev, meta_dev_idx] is
- * not yet used by some other drbd minor!
- * (if you use drbd.conf + drbdadm,
- * that should check it for you already; but if you don't, or someone
- * fooled it, we need to double check here) */
- nbc->md_bdev = inode2->i_bdev;
- if (bd_claim(nbc->md_bdev, (nbc->dc.meta_dev_idx < 0) ? (void *)mdev
- : (void *) drbd_m_holder)) {
- retcode = ERR_BDCLAIM_MD_DISK;
- goto release_bdev_fail;
- }
-
- if ((nbc->backing_bdev == nbc->md_bdev) !=
- (nbc->dc.meta_dev_idx == DRBD_MD_INDEX_INTERNAL ||
- nbc->dc.meta_dev_idx == DRBD_MD_INDEX_FLEX_INT)) {
- retcode = ERR_MD_IDX_INVALID;
- goto release_bdev2_fail;
+ goto fail;
}
/* RT - for drbd_get_max_capacity() DRBD_MD_INDEX_FLEX_INT */
@@ -982,7 +955,7 @@ static int drbd_nl_disk_conf(struct drbd_conf *mdev, struct drbd_nl_cfg_req *nlp
(unsigned long long) drbd_get_max_capacity(nbc),
(unsigned long long) nbc->dc.disk_size);
retcode = ERR_DISK_TO_SMALL;
- goto release_bdev2_fail;
+ goto fail;
}
if (nbc->dc.meta_dev_idx < 0) {
@@ -999,7 +972,7 @@ static int drbd_nl_disk_conf(struct drbd_conf *mdev, struct drbd_nl_cfg_req *nlp
dev_warn(DEV, "refusing attach: md-device too small, "
"at least %llu sectors needed for this meta-disk type
",
(unsigned long long) min_md_device_sectors);
- goto release_bdev2_fail;
+ goto fail;
}
/* Make sure the new disk is big enough
@@ -1007,7 +980,7 @@ static int drbd_nl_disk_conf(struct drbd_conf *mdev, struct drbd_nl_cfg_req *nlp
if (drbd_get_max_capacity(nbc) <
drbd_get_capacity(mdev->this_bdev)) {
retcode = ERR_DISK_TO_SMALL;
- goto release_bdev2_fail;
+ goto fail;
}
set_capacity(pd->disk, lba << 2);
@@ -2314,7 +2311,7 @@ static int pkt_open_dev(struct pktcdvd_device *pd, fmode_t write)
q = bdev_get_queue(pd->bdev);
if (write) {
if ((ret = pkt_open_write(pd)))
- goto out_unclaim;
+ goto out_putdev;
/*
* Some CDRW drives can not handle writes larger than one packet,
* even if the size is a multiple of the packet size.
@@ -2329,23 +2326,21 @@ static int pkt_open_dev(struct pktcdvd_device *pd, fmode_t write)
}
if (write) {
if (!pkt_grow_pktlist(pd, CONFIG_CDROM_PKTCDVD_BUFFERS)) {
printk(DRIVER_NAME": not enough memory for buffers
");
ret = -ENOMEM;
- goto out_unclaim;
+ goto out_putdev;
}
printk(DRIVER_NAME": %lukB available on disc
", lba << 1);
}
bdev = bdget_disk(block->gdp, 0);
- if (!bdev || blkdev_get(bdev, FMODE_READ) < 0)
+ if (!bdev || blkdev_get(bdev, FMODE_READ, NULL) < 0)
return -ENODEV;
/*
* See fs/partition/check.c:register_disk,rescan_partitions
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9329068..fc48912 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -660,7 +660,7 @@ static bool bd_may_claim(struct block_device *bdev, struct block_device *whole,
else if (bdev->bd_contains == bdev)
return true; /* is a whole device which isn't held */
- else if (whole->bd_holder == bd_claim)
+ else if (whole->bd_holder == bd_may_claim)
return true; /* is a partition of a device that is being partitioned */
else if (whole->bd_holder != NULL)
return false; /* is a partition of a held device */
@@ -807,10 +807,10 @@ static void __bd_claim(struct block_device *bdev, struct block_device *whole,
{
/* note that for a whole device bd_holders
* will be incremented twice, and bd_holder will
- * be set to bd_claim before being set to holder
+ * be set to bd_may_claim before being set to holder
*/
whole->bd_holders++;
- whole->bd_holder = bd_claim;
+ whole->bd_holder = bd_may_claim;
bdev->bd_holders++;
bdev->bd_holder = holder;
}
@@ -835,37 +835,7 @@ static void bd_finish_claiming(struct block_device *bdev,
__bd_abort_claiming(whole, holder); /* not actually an abort */
}
-/**
- * bd_claim - claim a block device
- * @bdev: block device to claim
- * @holder: holder trying to claim @bdev
- *
- * Try to claim @bdev which must have been opened successfully.
- *
- * CONTEXT:
- * Might sleep.
- *
- * RETURNS:
- * 0 if successful, -EBUSY if @bdev is already claimed.
- */
-int bd_claim(struct block_device *bdev, void *holder)
-{
- struct block_device *whole = bdev->bd_contains;
- int res;
-
- might_sleep();
-
- spin_lock(&bdev_lock);
- res = bd_prepare_to_claim(bdev, whole, holder);
- if (res == 0)
- __bd_claim(bdev, whole, holder);
- spin_unlock(&bdev_lock);
-
- return res;
-}
-EXPORT_SYMBOL(bd_claim);
-
-void bd_release(struct block_device *bdev)
+static void bd_release(struct block_device *bdev)
{
spin_lock(&bdev_lock);
if (!--bdev->bd_contains->bd_holders)
@@ -875,8 +845,6 @@ void bd_release(struct block_device *bdev)
spin_unlock(&bdev_lock);
}
-/**
- * close_bdev_exclusive - close a blockdevice opened by open_bdev_exclusive()
- *
- * @bdev: blockdevice to close
- * @mode: mode, must match that used to open.
- *
- * This is the counterpart to open_bdev_exclusive().
- */
-void close_bdev_exclusive(struct block_device *bdev, fmode_t mode)
-{
- bd_release(bdev);
- blkdev_put(bdev, mode);
-}
-
-EXPORT_SYMBOL(close_bdev_exclusive);
-
int __invalidate_device(struct block_device *bdev)
{
struct super_block *sb = get_super(bdev);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d395962..f1b729d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -489,7 +489,7 @@ again:
continue;
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index e1b8493..5a290f2 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -1120,16 +1120,13 @@ int lmLogOpen(struct super_block *sb)
* file systems to log may have n-to-1 relationship;
*/
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
11-03-2010, 03:10 PM
Christoph Hellwig
block: make blkdev_get/put() handle exclusive access
On Mon, Nov 01, 2010 at 05:15:28PM +0100, Tejun Heo wrote:
> * blkdev_get() is extended to include exclusive access management.
> @holder argument is added and, if is @FMODE_EXCL specified, it will
> gain exclusive access atomically w.r.t. other exclusive accesses.
>
> * blkdev_put() is similarly extended. It now takes @mode argument and
> if @FMODE_EXCL is set, it releases an exclusive access. Also, when
> the last exclusive claim is released, the holder/slave symlinks are
> removed automatically.
Could we get rid of FMODE_EXCL and just make a non-NULL holder field
mean to open it exlusively (and pass a holder to the blkdev_put to
release it)?
> * bd_link_disk_holder() remains the same but bd_unlink_disk_holder()
> is no longer necessary and removed.
That's a rather asymetric interface. What about having
blkdev_get_stacked that require a gendisk as holder and set up the
links underneath?
> open_bdev_exclusive() and open_by_devnum() can use further cleanup -
> rename to blkdev_get_by_path() and blkdev_get_by_devt() and drop
> special features. Well, let's leave them for another day.
s/blkdev_get_by_devt/blkdev_get_by_dev/
And yes, that rename is a good idea and should go in ASAP after this
patch.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
11-04-2010, 02:04 PM
Mike Snitzer
block: make blkdev_get/put() handle exclusive access
On Wed, Nov 03 2010 at 12:10pm -0400,
Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 01, 2010 at 05:15:28PM +0100, Tejun Heo wrote:
> > * blkdev_get() is extended to include exclusive access management.
> > @holder argument is added and, if is @FMODE_EXCL specified, it will
> > gain exclusive access atomically w.r.t. other exclusive accesses.
> >
> > * blkdev_put() is similarly extended. It now takes @mode argument and
> > if @FMODE_EXCL is set, it releases an exclusive access. Also, when
> > the last exclusive claim is released, the holder/slave symlinks are
> > removed automatically.
>
> Could we get rid of FMODE_EXCL and just make a non-NULL holder field
> mean to open it exlusively (and pass a holder to the blkdev_put to
> release it)?
I agree that the need for a FMODE_EXCL flag is awkward. Christoph's
proposed change should clean things up nicely.
Other than that, for the DM bits:
Acked-by: Mike Snitzer <snitzer@redhat.com>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
11-09-2010, 09:18 AM
Tejun Heo
block: make blkdev_get/put() handle exclusive access
Hello,
On 11/03/2010 04:06 PM, Jan Kara wrote:
> On Mon 01-11-10 17:15:28, Tejun Heo wrote:
> The patch looks OK to me as far as ext3, ext4, and reiserfs are
> concerned. One thing I wondered about when I looked at it - does someone
> use the 'mode' argument of the blkdev_put() function (well, apart from the
> exclusive flag)? Because I've looked at a few random disk ->release()
> functions and none of them used it...
Yeah, I thought about removing @mode and make blkdev_put() take
@holder for exclusive releases. From what I can see, it doesn't seem
like any implementation cares but it's customary to have access to
@mode on ->release(), so I decided to keep it. It's rather flaky as
there's no mechanism to check or enforce it tho.
Thanks.
--
tejun
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
11-09-2010, 09:34 AM
Tejun Heo
block: make blkdev_get/put() handle exclusive access
Hello,
On 11/03/2010 05:10 PM, Christoph Hellwig wrote:
> On Mon, Nov 01, 2010 at 05:15:28PM +0100, Tejun Heo wrote:
>> * blkdev_get() is extended to include exclusive access management.
>> @holder argument is added and, if is @FMODE_EXCL specified, it will
>> gain exclusive access atomically w.r.t. other exclusive accesses.
>>
>> * blkdev_put() is similarly extended. It now takes @mode argument and
>> if @FMODE_EXCL is set, it releases an exclusive access. Also, when
>> the last exclusive claim is released, the holder/slave symlinks are
>> removed automatically.
>
> Could we get rid of FMODE_EXCL and just make a non-NULL holder field
> mean to open it exlusively (and pass a holder to the blkdev_put to
> release it)?
Yeah, I agree it's a bit awkward. I'd really like to force one way or
the other tho. ie. if non-NULL holder is gonna be required, I'll add
WARN_ON_ONCE(mode & FMODE_EXCL). There are several issues to
consider.
* As Jan suggested, @mode in blkdev_put() isn't too useful. I decided
to keep it and use FMODE_EXCL for exclusive releases as that it is
at least useful for something. If we're gonna add @holder to
blkdev_put(), it would make more sense to drop @mode. It's not like
there's a way to enforce restrictions according to open @mode during
device access if there are mixed r/w opens.
* Some users don't keep @holder easily accessible until blkdev_put()
is called, so the conversion will take a bit more effort. No big
deal in itself.
* What if @holder on blkdev_put() mismatches the current holder?
Probably WARN_ON_ONCE() is the only recourse. At that point, it's a
bit silly to have to keep @holder around till blkdev_put(). Holders
and opners counting already provide meaningful warning mechanism
against spurious or missing exclusive releases. Maybe we can have
blkdev_put() and blkdev_put_exclusive()? Or make it take boolean
@excl?
So, after the above points, I decided to keep @mode. It is a bit
awkward but the other way didn't seem too hip either. Any better
ideas?
>> * bd_link_disk_holder() remains the same but bd_unlink_disk_holder()
>> is no longer necessary and removed.
>
> That's a rather asymetric interface. What about having
> blkdev_get_stacked that require a gendisk as holder and set up the
> links underneath?
That will make the number of functions multiplied by two -
blkdev_get_by_path_stacked() and blkdev_get_by_dev_stacked(). The
symlinking for stacked drivers is an oddball feature which is and will
be only used by md and dm. So, I think it's better to keep it
separate and oddball.
>> open_bdev_exclusive() and open_by_devnum() can use further cleanup -
>> rename to blkdev_get_by_path() and blkdev_get_by_devt() and drop
>> special features. Well, let's leave them for another day.
>
> s/blkdev_get_by_devt/blkdev_get_by_dev/
>
> And yes, that rename is a good idea and should go in ASAP after this
> patch.
Alright, will do it.
Thanks.
--
tejun
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
11-09-2010, 09:36 AM
Christoph Hellwig
block: make blkdev_get/put() handle exclusive access
On Tue, Nov 09, 2010 at 11:34:06AM +0100, Tejun Heo wrote:
> So, after the above points, I decided to keep @mode. It is a bit
> awkward but the other way didn't seem too hip either. Any better
> ideas?
Let's keep it for now and revisit it later.
> That will make the number of functions multiplied by two -
> blkdev_get_by_path_stacked() and blkdev_get_by_dev_stacked(). The
> symlinking for stacked drivers is an oddball feature which is and will
> be only used by md and dm. So, I think it's better to keep it
> separate and oddball.
Oh well. I still don't like it, but let keeps it simple to make
progress for now.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel