[PATCH] Forcing capacity sizes for block devices

[PATCH] Forcing capacity sizes for block devices

[Clemens Fruhwirth]

Make the size attribute of block devices writable via sysfs. This
allows userspace to force
device boundaries in cases where the device driver is unable to detect
the correct size, as
with spec-incompliant SD card readers. Also see
http://marc.info/?t=125555550200010&r=1&w=2

diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff
linux-2.6.32-vanilla/block/genhd.c linux-2.6.32-patched/block/genhd.c
--- linux-2.6.32-vanilla/block/genhd.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32-patched/block/genhd.c	2009-12-15 14:38:24.962463446 +0100
@@ -865,7 +865,7 @@ static DEVICE_ATTR(range, S_IRUGO, disk_
 static DEVICE_ATTR(ext_range, S_IRUGO, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, S_IRUGO, disk_removable_show, NULL);
 static DEVICE_ATTR(ro, S_IRUGO, disk_ro_show, NULL);
-static DEVICE_ATTR(size, S_IRUGO, part_size_show, NULL);
+static DEVICE_ATTR(size, S_IRUGO | S_IWUSR, part_size_show, part_size_set);
 static DEVICE_ATTR(alignment_offset, S_IRUGO,
disk_alignment_offset_show, NULL);
 static DEVICE_ATTR(capability, S_IRUGO, disk_capability_show, NULL);
 static DEVICE_ATTR(stat, S_IRUGO, part_stat_show, NULL);
diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff
linux-2.6.32-vanilla/fs/partitions/check.c
linux-2.6.32-patched/fs/partitions/check.c
--- linux-2.6.32-vanilla/fs/partitions/check.c	2009-12-03
04:51:21.000000000 +0100
+++ linux-2.6.32-patched/fs/partitions/check.c	2009-12-19
10:38:58.745294907 +0100
@@ -219,6 +219,18 @@ ssize_t part_size_show(struct device *de
 	return sprintf(buf, "%llu\n",(unsigned long long)p->nr_sects);
 }

+ssize_t part_size_set(struct device *dev,
+		      struct device_attribute *attr, char *buf, size_t len)
+{
+	struct hd_struct *p = dev_to_part(dev);
+	unsigned long long val;
+	if (strict_strtoull(buf, 0, &val))
+		return -EINVAL;
+
+	p->nr_sects = val;
+	return len;
+}
+
 ssize_t part_alignment_offset_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff
linux-2.6.32-vanilla/include/linux/genhd.h
linux-2.6.32-patched/include/linux/genhd.h
--- linux-2.6.32-vanilla/include/linux/genhd.h	2009-12-03
04:51:21.000000000 +0100
+++ linux-2.6.32-patched/include/linux/genhd.h	2009-12-19
10:40:30.900871273 +0100
@@ -549,6 +549,9 @@ extern void blk_unregister_region(dev_t

 extern ssize_t part_size_show(struct device *dev,
 			      struct device_attribute *attr, char *buf);
+extern ssize_t part_size_set(struct device *dev,
+			     struct device_attribute *attr, char *buf,
+			     size_t len);
 extern ssize_t part_stat_show(struct device *dev,
 			      struct device_attribute *attr, char *buf);
 extern ssize_t part_inflight_show(struct device *dev,

Signed-off-by: Clemens Fruhwirth <clemens@endorphin.org>
Reviewed-by: Grant Grundler <grundler@google.com>

Re: [PATCH] Forcing capacity sizes for block devices - (sorry, gmail ruined inlining with wordwrapping in the last mail)

[Clemens Fruhwirth]

Make the size attribute of block devices writable via sysfs. This allows 
userspace to force
device boundaries in cases where the device driver is unable to detect 
the correct size, as
with spec-incompliant SD card readers.

diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff 
linux-2.6.32-vanilla/block/genhd.c linux-2.6.32-patched/block/genhd.c
--- linux-2.6.32-vanilla/block/genhd.c    2009-12-03 04:51:21.000000000 
+0100
+++ linux-2.6.32-patched/block/genhd.c    2009-12-15 14:38:24.962463446 
+0100
@@ -865,7 +865,7 @@ static DEVICE_ATTR(range, S_IRUGO, disk_
  static DEVICE_ATTR(ext_range, S_IRUGO, disk_ext_range_show, NULL);
  static DEVICE_ATTR(removable, S_IRUGO, disk_removable_show, NULL);
  static DEVICE_ATTR(ro, S_IRUGO, disk_ro_show, NULL);
-static DEVICE_ATTR(size, S_IRUGO, part_size_show, NULL);
+static DEVICE_ATTR(size, S_IRUGO | S_IWUSR, part_size_show, 
part_size_set);
  static DEVICE_ATTR(alignment_offset, S_IRUGO, 
disk_alignment_offset_show, NULL);
  static DEVICE_ATTR(capability, S_IRUGO, disk_capability_show, NULL);
  static DEVICE_ATTR(stat, S_IRUGO, part_stat_show, NULL);
diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff 
linux-2.6.32-vanilla/fs/partitions/check.c 
linux-2.6.32-patched/fs/partitions/check.c
--- linux-2.6.32-vanilla/fs/partitions/check.c    2009-12-03 
04:51:21.000000000 +0100
+++ linux-2.6.32-patched/fs/partitions/check.c    2009-12-19 
10:38:58.745294907 +0100
@@ -219,6 +219,18 @@ ssize_t part_size_show(struct device *de
      return sprintf(buf, "%llu\n",(unsigned long long)p->nr_sects);
  }

+ssize_t part_size_set(struct device *dev,
+              struct device_attribute *attr, char *buf, size_t len)
+{
+    struct hd_struct *p = dev_to_part(dev);
+    unsigned long long val;
+    if (strict_strtoull(buf, 0, &val))
+        return -EINVAL;
+
+    p->nr_sects = val;
+    return len;
+}
+
  ssize_t part_alignment_offset_show(struct device *dev,
                     struct device_attribute *attr, char *buf)
  {
diff -uprN -X linux-2.6.32-vanilla/Documentation/dontdiff 
linux-2.6.32-vanilla/include/linux/genhd.h 
linux-2.6.32-patched/include/linux/genhd.h
--- linux-2.6.32-vanilla/include/linux/genhd.h    2009-12-03 
04:51:21.000000000 +0100
+++ linux-2.6.32-patched/include/linux/genhd.h    2009-12-19 
10:40:30.900871273 +0100
@@ -549,6 +549,9 @@ extern void blk_unregister_region(dev_t

  extern ssize_t part_size_show(struct device *dev,
                    struct device_attribute *attr, char *buf);
+extern ssize_t part_size_set(struct device *dev,
+                 struct device_attribute *attr, char *buf,
+                 size_t len);
  extern ssize_t part_stat_show(struct device *dev,
                    struct device_attribute *attr, char *buf);
  extern ssize_t part_inflight_show(struct device *dev,

Signed-off-by: Clemens Fruhwirth <clemens@endorphin.org>
Reviewed-by: Grant Grundler <grundler@google.com>

Re: [PATCH] Forcing capacity sizes for block devices

[Bartlomiej Zolnierkiewicz]

On Sunday 20 December 2009 01:05:35 pm Clemens Fruhwirth wrote:
> Make the size attribute of block devices writable via sysfs. This
> allows userspace to force
> device boundaries in cases where the device driver is unable to detect
> the correct size, as
> with spec-incompliant SD card readers. Also see
> http://marc.info/?t=125555550200010&r=1&w=2

Just a wild idea but maybe the current block infrastructure for handling
ATA HPA (see commit db429e9 and e957b60 one) can be used to automatically
detect and workaround the issue?  
--
Bartlomiej Zolnierkiewicz

Re: [PATCH] Forcing capacity sizes for block devices

[Grant Grundler]

On Sun, Dec 20, 2009 at 9:43 AM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
> On Sunday 20 December 2009 01:05:35 pm Clemens Fruhwirth wrote:
>> Make the size attribute of block devices writable via sysfs. This
>> allows userspace to force
>> device boundaries in cases where the device driver is unable to detect
>> the correct size, as
>> with spec-incompliant SD card readers. Also see
>> http://marc.info/?t=125555550200010&r=1&w=2
>
> Just a wild idea but maybe the current block infrastructure for handling
> ATA HPA (see commit db429e9 and e957b60 one) can be used to automatically
> detect and workaround the issue?

Are you referring to this change?
+       libata.ignore_hpa=      [LIBATA] Ignore HPA limit
+                       libata.ignore_hpa=0       keep BIOS limits (default)
+                       libata.ignore_hpa=1       ignore limits, using full disk

My git-foo isn't very good...what command will display the two changes
you refer to above?

Looking at latest drivers/ata/libata-core.c where ignore_hpa is used,
that could be leveraged. Clemens will have to comment on if there is a
"detect" method that would work for his problem.

Secondly, the "ignore_hpa" approach is orthogonal to allowing user space
to override the partition size.  This patch does not prevent the kernel from
attempting to fix up the partition/block size. If the kernel can't or
doesn't know
about b0rked HW, the user (or udev maybe?) can workaround this issue.
Given how simple the patch, I don't see any reason to reject it.

thanks,
grant

Re: [PATCH] Forcing capacity sizes for block devices

[Bartlomiej Zolnierkiewicz]

On Monday 21 December 2009 09:04:03 pm Grant Grundler wrote:
> On Sun, Dec 20, 2009 at 9:43 AM, Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> wrote:
> > On Sunday 20 December 2009 01:05:35 pm Clemens Fruhwirth wrote:
> >> Make the size attribute of block devices writable via sysfs. This
> >> allows userspace to force
> >> device boundaries in cases where the device driver is unable to detect
> >> the correct size, as
> >> with spec-incompliant SD card readers. Also see
> >> http://marc.info/?t=125555550200010&r=1&w=2
> >
> > Just a wild idea but maybe the current block infrastructure for handling
> > ATA HPA (see commit db429e9 and e957b60 one) can be used to automatically
> > detect and workaround the issue?
> 
> Are you referring to this change?
> +       libata.ignore_hpa=      [LIBATA] Ignore HPA limit
> +                       libata.ignore_hpa=0       keep BIOS limits (default)
> +                       libata.ignore_hpa=1       ignore limits, using full disk

Nah.

> My git-foo isn't very good...what command will display the two changes
> you refer to above?

git log -p --color db429e9^1..e957b60

We should be able to detect working SD reader/card combination in sd
driver's ->set_capacity method implementation (i.e. by issuing read
request for the last partitioned sector outside of the reported device
capacity) during the initial partition table scan and at the same time
adjust the device capacity used by the kernel if necessary.

New/empty cards would still need to be 'force partitioned' first using
fdisk (or sfdisk if fdisk doesn't allow such operation) but after it's
done the partition rescan on fdisk's exit will recognize new device's
capacity automatically..

> about b0rked HW, the user (or udev maybe?) can workaround this issue.
> Given how simple the patch, I don't see any reason to reject it.

Simple to write or simple to maintain? ;)

To be honest the patch looks like a gross and dangerous hack..

Firstly block layer needs to be properly informed about capacity changes
(set_capacity() + request for partition rescan which the patch doesn't do)
and secondly fixing the problem at the sysfs block/partition layer seems
to be a layering violation (i.e. it also allows partition size changes).

--
Bartlomiej Zolnierkiewicz

Re: [PATCH] Forcing capacity sizes for block devices

[Grant Grundler]

On Mon, Dec 21, 2009 at 1:59 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
...
>> > Just a wild idea but maybe the current block infrastructure for handling
>> > ATA HPA (see commit db429e9 and e957b60 one) can be used to automatically
>> > detect and workaround the issue?
>>
>> Are you referring to this change?
>> +       libata.ignore_hpa=      [LIBATA] Ignore HPA limit
>> +                       libata.ignore_hpa=0       keep BIOS limits (default)
>> +                       libata.ignore_hpa=1       ignore limits, using full disk
>
> Nah.
>
>> My git-foo isn't very good...what command will display the two changes
>> you refer to above?
>
> git log -p --color db429e9^1..e957b60

Perfect!

> We should be able to detect working SD reader/card combination in sd
> driver's ->set_capacity method implementation (i.e. by issuing read
> request for the last partitioned sector outside of the reported device
> capacity) during the initial partition table scan and at the same time
> adjust the device capacity used by the kernel if necessary.

Yup - I think you are right.

> New/empty cards would still need to be 'force partitioned' first using
> fdisk (or sfdisk if fdisk doesn't allow such operation) but after it's
> done the partition rescan on fdisk's exit will recognize new device's
> capacity automatically..
>
>> about b0rked HW, the user (or udev maybe?) can workaround this issue.
>> Given how simple the patch, I don't see any reason to reject it.
>
> Simple to write or simple to maintain? ;)

Write of course. :)
And in retrospect, even that statement is wrong since nothing in the
storage stack is as simple as it looks.

> To be honest the patch looks like a gross and dangerous hack..

Agreed.  I expect there will always be some devices that will get the
partition/device size wrong and it would be expedient use this hack to
work around "non-compliant" devices. Clemens already proved it works
for him (even though it's not quite right).

> Firstly block layer needs to be properly informed about capacity changes
> (set_capacity() + request for partition rescan which the patch doesn't do)
> and secondly fixing the problem at the sysfs block/partition layer seems
> to be a layering violation (i.e. it also allows partition size changes).

It's the exported interface to user space. So while it's not
implemented correctly to deal with resizing (which I didn't realize,
Clemens will have to fix that), I don't consider it a layering
violation if there are APIs this code can call to notify of the
partition size change.

thanks!
grant


>
> --
> Bartlomiej Zolnierkiewicz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Forcing capacity sizes for block devices

[Clemens Fruhwirth]

Hi Bartlomiej, Hi Grant!

(sorry for resending this mail, gmail switched into HTML mode for
unknown reasons, and vger.kernel.org rightfully rejected this mail)

On Mon, Dec 21, 2009 at 10:59 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>
> We should be able to detect working SD reader/card combination in sd
> driver's ->set_capacity method implementation (i.e. by issuing read
> request for the last partitioned sector outside of the reported device
> capacity) during the initial partition table scan and at the same time
> adjust the device capacity used by the kernel if necessary.

1. I disagree with the strategy to add some automatism that does not
work for all cases (unformated cards in this case). Do you want the
user to jump through the additional hop of creating oversized
partition tables that fdisk and friends might not even allow easily?
The case with manual override is easier and more straight forward.
Also this approach is tied to using partition tables and in my case SD
cards work fine without partition tables.

2. I strongly opt for not-creating a kernel that tries to domineer[1]
over its users. If I say my device is size X I want the kernel to
follow that order. Also I dislike the kernel to probe beyond the
device boundaries unless I told it do so. I don't want to be this
behavior to be auto enabled by (probably broken) partition tables that
I can not even see before plugging that disk into my device (and
naturally triggering this behavior)

3. The kernel is not supposed to work correctly when writing incorrect
junk to random sysfs files. So assuming the required knowledge for
manual intervention is ok at /sys.

> Firstly block layer needs to be properly informed about capacity changes
> (set_capacity() + request for partition rescan which the patch doesn't do)
> and secondly fixing the problem at the sysfs block/partition layer seems
> to be a layering violation (i.e. it also allows partition size changes).

git log -p --color db429e9^1..e957b60 does not seem related to me. The
implementation of ide_disk_set_capacity suggests that the semantic of
set_capacity is that the caller can make the device smaller than
probed_capacity with the intention to protect stuff (Host Protect
Area?). The caller can not enlarge the device beyond the device
boundaries because of its first line "u64 set = min(capacity,
drive->probed_capacity)". The intention of my patch and its potential
use of any set_capacity is exactly that, namely to override these
device limits. So it's seems a bit orthogonal.

I don't object to inform the device layer somehow about the forceful
size change, however is set_capacity really the intended place to do
it? Where do we set nr_sects, if so?

[1] Sorry if that translation sounds strange:
http://dict.leo.org/ende?search=bevormunden
--
Fruhwirth Clemens http://clemens.endorphin.org

Re: [PATCH] Forcing capacity sizes for block devices

[Clemens Fruhwirth]

Can we resume this thread? I still have objections against an
automated solution (see below) and would like to get a patch merged
that makes "size" writable in sysfs. We should probably think of a way
to inform the block layer more formally.

On Thu, Dec 24, 2009 at 2:40 PM, Clemens Fruhwirth
<clemens@endorphin.org> wrote:
> Hi Bartlomiej, Hi Grant!
>
> On Mon, Dec 21, 2009 at 10:59 PM, Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> wrote:
>>
>> We should be able to detect working SD reader/card combination in sd
>> driver's ->set_capacity method implementation (i.e. by issuing read
>> request for the last partitioned sector outside of the reported device
>> capacity) during the initial partition table scan and at the same time
>> adjust the device capacity used by the kernel if necessary.
>
> 1. I disagree with the strategy to add some automatism that does not
> work for all cases (unformated cards in this case). Do you want the
> user to jump through the additional hop of creating oversized
> partition tables that fdisk and friends might not even allow easily?
> The case with manual override is easier and more straight forward.
> Also this approach is tied to using partition tables and in my case SD
> cards work fine without partition tables.
>
> 2. I strongly opt for not-creating a kernel that tries to domineer[1]
> over its users. If I say my device is size X I want the kernel to
> follow that order. Also I dislike the kernel to probe beyond the
> device boundaries unless I told it do so. I don't want to be this
> behavior to be auto enabled by (probably broken) partition tables that
> I can not even see before plugging that disk into my device (and
> naturally triggering this behavior)
>
> 3. The kernel is not supposed to work correctly when writing incorrect
> junk to random sysfs files. So assuming the required knowledge for
> manual intervention is ok at /sys.
>
>> Firstly block layer needs to be properly informed about capacity changes
>> (set_capacity() + request for partition rescan which the patch doesn't do)
>> and secondly fixing the problem at the sysfs block/partition layer seems
>> to be a layering violation (i.e. it also allows partition size changes).
>
> git log -p --color db429e9^1..e957b60 does not seem related to me. The
> implementation of ide_disk_set_capacity suggests that the semantic of
> set_capacity is that the caller can make the device smaller than
> probed_capacity with the intention to protect stuff (Host Protect
> Area?). The caller can not enlarge the device beyond the device
> boundaries because of its first line "u64 set = min(capacity,
> drive->probed_capacity)". The intention of my patch and its potential
> use of any set_capacity is exactly that, namely to override these
> device limits. So it's seems a bit orthogonal.
>
> I don't object to inform the device layer somehow about the forceful
> size change, however is set_capacity really the intended place to do
> it? Where do we set nr_sects, if so?
>
> [1] Sorry if that translation sounds strange:
> http://dict.leo.org/ende?search=bevormunden
> --
> Fruhwirth Clemens http://clemens.endorphin.org
>
-- 
Fruhwirth Clemens http://clemens.endorphin.org