On 07/09/2012 05:34 PM, vgoyal@redhat.com wrote:
> Phillip, do let me know if I should put your signed-off-by also in the
> patch.
Sure, kernel side looks good. My original util-linux patches also added a -u update mode to kpartx, which I think is the more useful interface than the lower level resizepart command, but I suppose I can rebase it to apply on top of this patch.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
07-10-2012, 01:57 PM
Vivek Goyal
block: Support online resize of disk partitions
On Mon, Jul 09, 2012 at 06:40:03PM -0400, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/09/2012 05:34 PM, vgoyal@redhat.com wrote:
> > Phillip, do let me know if I should put your signed-off-by also in the
> > patch.
>
> Sure, kernel side looks good. My original util-linux patches also added a -u update mode to kpartx, which I think is the more useful interface than the lower level resizepart command, but I suppose I can rebase it to apply on top of this patch.
I wrote user space part only to do testing. We definitely need better user
space support. Especially a utility which changes partition details both
in kernel as well as on disk. Asking user to call two different utilities
is error prone and irritating.
Resending this patch again with your Signed-off-by.
Add a new operation code (BLKPG_RESIZE_PARTITION) to the BLKPG ioctl that
allows altering the size of an existing partition, even if it is currently
in use.
This patch converts hd_struct->nr_sects into sequence counter because
One might extend a partition while IO is happening to it and update of
nr_sects can be non-atomic on 32bit machines with 64bit sector_t. This
can lead to issues like reading inconsistent size of a partition. Sequence
counter have been used so that readers don't have to take bdev mutex lock
as we call sector_in_part() very frequently.
Now all the access to hd_struct->nr_sects should happen using sequence
counter read/update helper functions part_nr_sects_read/part_nr_sects_write.
There is one exception though, set_capacity()/get_capacity(). I think
theoritically race should exist there too but this patch does not
modify set_capacity()/get_capacity() due to sheer number of call sites
and I am afraid that change might break something. I have left that as a
TODO item. We can handle it later if need be. This patch does not introduce
any new races as such w.r.t set_capacity()/get_capacity().
+ /*
+ * set_capacity() and get_capacity() currently don't use
+ * seqcounter to read/update the part0->nr_sects. Still init
+ * the counter as we can read the sectors in IO submission
+ * patch using seqence counters.
+ *
+ * TODO: Ideally set_capacity() and get_capacity() should be
+ * converted to make use of bd_mutex and sequence counters.
+ */
+ seqcount_init(&disk->part0.nr_sects_seq);
hd_ref_init(&disk->part0);
disk->minors = minors;
diff --git a/block/ioctl.c b/block/ioctl.c
index ba15b2d..4476e0e8 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -13,7 +13,7 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
{
struct block_device *bdevp;
struct gendisk *disk;
- struct hd_struct *part;
+ struct hd_struct *part, *lpart;
struct blkpg_ioctl_arg a;
struct blkpg_partition p;
struct disk_part_iter piter;
@@ -36,8 +36,8 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
case BLKPG_ADD_PARTITION:
start = p.start >> 9;
length = p.length >> 9;
- /* check for fit in a hd_struct */
- if (sizeof(sector_t) == sizeof(long) &&
+ /* check for fit in a hd_struct */
+ if (sizeof(sector_t) == sizeof(long) &&
sizeof(long long) > sizeof(long)) {
long pstart = start, plength = length;
if (pstart != start || plength != length
@@ -92,6 +92,59 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
bdput(bdevp);
p->start_sect = start;
diff --git a/include/linux/blkpg.h b/include/linux/blkpg.h
index faf8a45..a851944 100644
--- a/include/linux/blkpg.h
+++ b/include/linux/blkpg.h
@@ -40,6 +40,7 @@ struct blkpg_ioctl_arg {
/* The subfunctions (for the op field) */
#define BLKPG_ADD_PARTITION 1
#define BLKPG_DEL_PARTITION 2
+#define BLKPG_RESIZE_PARTITION 3
/* Sizes of name fields. Unused at present. */
#define BLKPG_DEVNAMELTH 64
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 017a7fb..ee8e688 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -98,7 +98,13 @@ struct partition_meta_info {
struct hd_struct {
sector_t start_sect;
+ /*
+ * nr_sects is protected by sequence counter. One might extend a
+ * partition while IO is happening to it and update of nr_sects
+ * can be non-atomic on 32bit machines with 64bit sector_t.
+ */
sector_t nr_sects;
+ seqcount_t nr_sects_seq;
sector_t alignment_offset;
unsigned int discard_alignment;
struct device __dev;
@@ -648,6 +654,57 @@ static inline void hd_struct_put(struct hd_struct *part)
__delete_partition(part);
}
+/*
+ * Any access of part->nr_sects which is not protected by partition
+ * bd_mutex or gendisk bdev bd_mutex, should be done using this
+ * accessor function.
+ *
+ * Code written along the lines of i_size_read() and i_size_write().
+ * CONFIG_PREEMPT case optimizes the case of UP kernel with preemption
+ * on.
+ */
+static inline sector_t part_nr_sects_read(struct hd_struct *part)
+{
+#if BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_SMP)
+ sector_t nr_sects;
+ unsigned seq;
+ do {
+ seq = read_seqcount_begin(&part->nr_sects_seq);
+ nr_sects = part->nr_sects;
+ } while (read_seqcount_retry(&part->nr_sects_seq, seq));
+ return nr_sects;
+#elif BITS_PER_LONG==32 && defined(CONFIG_PREEMPT)
+ sector_t nr_sects;
+
+ preempt_disable();
+ nr_sects = part->nr_sects;
+ preempt_enable();
+ return nr_sects;
+#else
+ return part->nr_sects;
+#endif
+}
+
+/*
+ * Should be called with mutex lock held (typically bd_mutex) of partition
+ * to provide mutual exlusion among writers otherwise seqcount might be
+ * left in wrong state leaving the readers spinning infinitely.
+ */
+static inline void part_nr_sects_write(struct hd_struct *part, sector_t size)
+{
+#if BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_SMP)
+ write_seqcount_begin(&part->nr_sects_seq);
+ part->nr_sects = size;
+ write_seqcount_end(&part->nr_sects_seq);
+#elif BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_PREEMPT)
+ preempt_disable();
+ part->nr_sects = size;
+ preempt_enable();
+#else
+ part->nr_sects = size;
+#endif
+}
+
#else /* CONFIG_BLOCK */
Shouldn't this be BITS_PER_LONG==32 && defined(CONFIG_LBDAF) &&
defined(CONFIG_PREEMPT)? No sense disabling preemption when the
sector size is also 32 bits.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
07-10-2012, 02:20 PM
Vivek Goyal
block: Support online resize of disk partitions
On Tue, Jul 10, 2012 at 10:13:19AM -0400, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 7/10/2012 9:57 AM, Vivek Goyal wrote:
> > +static inline sector_t part_nr_sects_read(struct hd_struct *part)
> > +{ +#if BITS_PER_LONG==32 && defined(CONFIG_LBDAF) &&
> > defined(CONFIG_SMP) + sector_t nr_sects; + unsigned seq; + do { +
> > seq = read_seqcount_begin(&part->nr_sects_seq); + nr_sects =
> > part->nr_sects; + } while (read_seqcount_retry(&part->nr_sects_seq,
> > seq)); + return nr_sects; +#elif BITS_PER_LONG==32 &&
> > defined(CONFIG_PREEMPT)
>
> Shouldn't this be BITS_PER_LONG==32 && defined(CONFIG_LBDAF) &&
> defined(CONFIG_PREEMPT)? No sense disabling preemption when the
> sector size is also 32 bits.
Yes. Good catch. We don't want to disable/enable preemption for 32bit UP
kernels with sector size 32bit. I will modify the patch and repost soon.
BTW, what happened to all the new lines in the code above. Looks like you
mailer chewed these up.
Thanks
Vivek
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
07-10-2012, 02:59 PM
Vivek Goyal
block: Support online resize of disk partitions
On Tue, Jul 10, 2012 at 10:20:01AM -0400, Vivek Goyal wrote:
[..]
> >
> > Shouldn't this be BITS_PER_LONG==32 && defined(CONFIG_LBDAF) &&
> > defined(CONFIG_PREEMPT)? No sense disabling preemption when the
> > sector size is also 32 bits.
>
> Yes. Good catch. We don't want to disable/enable preemption for 32bit UP
> kernels with sector size 32bit. I will modify the patch and repost soon.
>
> BTW, what happened to all the new lines in the code above. Looks like you
> mailer chewed these up.
Here is the V2 of the patch with CONFIG_LBDAF test added to UP, PREEMPT
case.
Thanks
Vivek
block: add partition resize function to blkpg ioctl
Add a new operation code (BLKPG_RESIZE_PARTITION) to the BLKPG ioctl that
allows altering the size of an existing partition, even if it is currently
in use.
This patch converts hd_struct->nr_sects into sequence counter because
One might extend a partition while IO is happening to it and update of
nr_sects can be non-atomic on 32bit machines with 64bit sector_t. This
can lead to issues like reading inconsistent size of a partition. Sequence
counter have been used so that readers don't have to take bdev mutex lock
as we call sector_in_part() very frequently.
Now all the access to hd_struct->nr_sects should happen using sequence
counter read/update helper functions part_nr_sects_read/part_nr_sects_write.
There is one exception though, set_capacity()/get_capacity(). I think
theoritically race should exist there too but this patch does not
modify set_capacity()/get_capacity() due to sheer number of call sites
and I am afraid that change might break something. I have left that as a
TODO item. We can handle it later if need be. This patch does not introduce
any new races as such w.r.t set_capacity()/get_capacity().
v2: Add CONFIG_LBDAF test to UP preempt case as suggested by Phillip.
+ /*
+ * set_capacity() and get_capacity() currently don't use
+ * seqcounter to read/update the part0->nr_sects. Still init
+ * the counter as we can read the sectors in IO submission
+ * patch using seqence counters.
+ *
+ * TODO: Ideally set_capacity() and get_capacity() should be
+ * converted to make use of bd_mutex and sequence counters.
+ */
+ seqcount_init(&disk->part0.nr_sects_seq);
hd_ref_init(&disk->part0);
disk->minors = minors;
Index: linux-2.6/block/ioctl.c
================================================== =================
--- linux-2.6.orig/block/ioctl.c 2012-07-10 21:03:52.459190498 -0400
+++ linux-2.6/block/ioctl.c 2012-07-10 21:03:57.067190639 -0400
@@ -13,7 +13,7 @@ static int blkpg_ioctl(struct block_devi
{
struct block_device *bdevp;
struct gendisk *disk;
- struct hd_struct *part;
+ struct hd_struct *part, *lpart;
struct blkpg_ioctl_arg a;
struct blkpg_partition p;
struct disk_part_iter piter;
@@ -36,8 +36,8 @@ static int blkpg_ioctl(struct block_devi
case BLKPG_ADD_PARTITION:
start = p.start >> 9;
length = p.length >> 9;
- /* check for fit in a hd_struct */
- if (sizeof(sector_t) == sizeof(long) &&
+ /* check for fit in a hd_struct */
+ if (sizeof(sector_t) == sizeof(long) &&
sizeof(long long) > sizeof(long)) {
long pstart = start, plength = length;
if (pstart != start || plength != length
@@ -92,6 +92,59 @@ static int blkpg_ioctl(struct block_devi
bdput(bdevp);
struct hd_struct {
sector_t start_sect;
+ /*
+ * nr_sects is protected by sequence counter. One might extend a
+ * partition while IO is happening to it and update of nr_sects
+ * can be non-atomic on 32bit machines with 64bit sector_t.
+ */
sector_t nr_sects;
+ seqcount_t nr_sects_seq;
sector_t alignment_offset;
unsigned int discard_alignment;
struct device __dev;
@@ -648,6 +654,57 @@ static inline void hd_struct_put(struct
__delete_partition(part);
}
+/*
+ * Any access of part->nr_sects which is not protected by partition
+ * bd_mutex or gendisk bdev bd_mutex, should be done using this
+ * accessor function.
+ *
+ * Code written along the lines of i_size_read() and i_size_write().
+ * CONFIG_PREEMPT case optimizes the case of UP kernel with preemption
+ * on.
+ */
+static inline sector_t part_nr_sects_read(struct hd_struct *part)
+{
+#if BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_SMP)
+ sector_t nr_sects;
+ unsigned seq;
+ do {
+ seq = read_seqcount_begin(&part->nr_sects_seq);
+ nr_sects = part->nr_sects;
+ } while (read_seqcount_retry(&part->nr_sects_seq, seq));
+ return nr_sects;
+#elif BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_PREEMPT)
+ sector_t nr_sects;
+
+ preempt_disable();
+ nr_sects = part->nr_sects;
+ preempt_enable();
+ return nr_sects;
+#else
+ return part->nr_sects;
+#endif
+}
+
+/*
+ * Should be called with mutex lock held (typically bd_mutex) of partition
+ * to provide mutual exlusion among writers otherwise seqcount might be
+ * left in wrong state leaving the readers spinning infinitely.
+ */
+static inline void part_nr_sects_write(struct hd_struct *part, sector_t size)
+{
+#if BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_SMP)
+ write_seqcount_begin(&part->nr_sects_seq);
+ part->nr_sects = size;
+ write_seqcount_end(&part->nr_sects_seq);
+#elif BITS_PER_LONG==32 && defined(CONFIG_LBDAF) && defined(CONFIG_PREEMPT)
+ preempt_disable();
+ part->nr_sects = size;
+ preempt_enable();
+#else
+ part->nr_sects = size;
+#endif
+}
+
#else /* CONFIG_BLOCK */
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
08-01-2012, 02:07 PM
Vivek Goyal
block: Support online resize of disk partitions
On Mon, Jul 09, 2012 at 05:34:18PM -0400, vivek.goyal2008@gmail.com wrote:
> Hi,
>
> Few people have pinged me in rencent past about status of this patch, hence,
> this is V4 of patch which adds support for online resizing of a partition.
> This patch is based on previously posted patches by Phillip Susi.
>
Hi Jens,
Can you please consider partition resize patches for inclusion.
Thanks
Vivek
> There are two patches. Out of which one is kernel patch and other one is
> util-linux patch to add support of a user space utility "resizepart" to
> allow resizing the partition.
>
> This ioctl only resizes the partition size in kenrel and does not change
> the size on disk. A user needs to make sure that corresponding changes
> are made to disk data structures also using fdisk(or partx), if changes
> are to be retained across reboot.
>
> Changes since V3
> ----------------
> - Do bdput() in error path as per the Maxim's review comments.
>
> Changes since V2
> ----------------
> - Do not ignore the "start" parameter in RESIZE ioctl.
> - Change resizepart utility to parse sysfs to get to partition start.
>
> Changes since V1
> ----------------
> Following are changes since the version Phillip posted.
> - RESIZE ioctl ignores the partition "start" and does not expect user to
> specify one. Caller needs to just specify "device", "partition number" and
> "size" of new partition.
>
> - Got rid of part_nr_sects_write_begin/part_nr_sects_write_end functions
> and replaced these with single part_nr_sects_write().
>
> - Some sequence counter related changes are simply lifted from i_size_write().
>
> - Initialized part->nr_sects_seq using seqcount_init().
>
> Phillip, do let me know if I should put your signed-off-by also in the
> patch.
>
> Any review feedback is welcome.
>
> I did following test.
>
> - Create a partition of 10MB on a disk using fdisk.
> - Add this partition to a volume group
> - Use fdisk to increase the partition size to 20MB. (First delete the
> partition and then create a new one of 20MB size).
> - Use resizepart to extend partition size in kernel.
> resizepart /dev/sdc 1 40960
> - Do pvresize on partition so that physical volume can be incrased in
> size online.
> pvresize /dev/sda1
>
> pvresize does recognize the new size. Also lsblk and /proc/partitions
> report the new size of partition.
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
08-01-2012, 03:49 PM
Jens Axboe
block: Support online resize of disk partitions
On 08/01/2012 04:07 PM, Vivek Goyal wrote:
> On Mon, Jul 09, 2012 at 05:34:18PM -0400, vivek.goyal2008@gmail.com wrote:
>> Hi,
>>
>> Few people have pinged me in rencent past about status of this patch, hence,
>> this is V4 of patch which adds support for online resizing of a partition.
>> This patch is based on previously posted patches by Phillip Susi.
>>
>
> Hi Jens,
>
> Can you please consider partition resize patches for inclusion.
Vivek, they are in and were in the pull request sent out earlier today!
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
08-01-2012, 03:59 PM
Vivek Goyal
block: Support online resize of disk partitions
On Wed, Aug 01, 2012 at 05:49:11PM +0200, Jens Axboe wrote:
> On 08/01/2012 04:07 PM, Vivek Goyal wrote:
> > On Mon, Jul 09, 2012 at 05:34:18PM -0400, vivek.goyal2008@gmail.com wrote:
> >> Hi,
> >>
> >> Few people have pinged me in rencent past about status of this patch, hence,
> >> this is V4 of patch which adds support for online resizing of a partition.
> >> This patch is based on previously posted patches by Phillip Susi.
> >>
> >
> > Hi Jens,
> >
> > Can you please consider partition resize patches for inclusion.
>
> Vivek, they are in and were in the pull request sent out earlier today!
Great. I missed that. Thanks for including the patch.
Vivek
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel