[resending]
Hi,
Currently the rescan_partition returns 0 (success), even if it is unable
to rescan the partition information ( may be due to disks offline, I/O
error reading the table or unknown partition ). This would make ioctl()
calls succeed for BLKRRPART requests even if partitions were not scanned
properly, which is not a good thing to do.
Attached here is patch to fix the issue. The patch makes
rescan_partition to return -EINVAL for unknown partitions and -EIO for
disk I/O errors ( or when disks are offline ).
Comments ?
Thanks,
Suzuki K P <[email protected]>
Linux Technology Centre,
IBM Systems & Technology Labs.
* Fix recscan_partitions to return error when the partition is unknown
or there is an I/O error reading the partition information.
Signed Off by : Suzuki K P <[email protected]>
Index: linux-2.6.18/fs/partitions/check.c
===================================================================
--- linux-2.6.18.orig/fs/partitions/check.c 2006-09-26
04:41:55.000000000 +0530
+++ linux-2.6.18/fs/partitions/check.c 2006-09-26 05:09:29.000000000 +0530
@@ -177,7 +177,7 @@
else if (warn_no_part)
printk(" unable to read partition table\n");
kfree(state);
- return NULL;
+ return ERR_PTR(res);
}
/*
@@ -458,8 +458,13 @@
delete_partition(disk, p);
if (disk->fops->revalidate_disk)
disk->fops->revalidate_disk(disk);
- if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
- return 0;
+ if (!get_capacity(disk))
+ return -EINVAL;
+ state = check_partition(disk, bdev);
+ if (!state)
+ return -EINVAL;
+ if (IS_ERR(state))
+ return -EIO;
for (p = 1; p < state->limit; p++) {
sector_t size = state->parts[p].size;
sector_t from = state->parts[p].from;
On Tue, Oct 03, 2006 at 06:00:36PM -0700, Suzuki Kp wrote:
> Currently the rescan_partition returns 0 (success), even if it is unable
> to rescan the partition information ( may be due to disks offline, I/O
> error reading the table or unknown partition ). This would make ioctl()
> calls succeed for BLKRRPART requests even if partitions were not scanned
> properly, which is not a good thing to do.
>
> Attached here is patch to fix the issue. The patch makes
> rescan_partition to return -EINVAL for unknown partitions and -EIO for
> disk I/O errors ( or when disks are offline ).
I don't think it's a good idea to return an error when there's an
unknown partition table. How do you differentiate between a device that
isn't partitioned at all and a device with an unknown partition table?
Better return 0 on an unknown partition table.
Erik
--
+-- Erik Mouw -- http://www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands
Erik Mouw wrote:
> On Tue, Oct 03, 2006 at 06:00:36PM -0700, Suzuki Kp wrote:
>
>>Currently the rescan_partition returns 0 (success), even if it is unable
>>to rescan the partition information ( may be due to disks offline, I/O
>>error reading the table or unknown partition ). This would make ioctl()
>>calls succeed for BLKRRPART requests even if partitions were not scanned
>>properly, which is not a good thing to do.
>>
>>Attached here is patch to fix the issue. The patch makes
>>rescan_partition to return -EINVAL for unknown partitions and -EIO for
>>disk I/O errors ( or when disks are offline ).
>
>
> I don't think it's a good idea to return an error when there's an
> unknown partition table. How do you differentiate between a device that
> isn't partitioned at all and a device with an unknown partition table?
Returning -EINVAL in both the above cases would inform the user that,
the partition table was not read properly due to an invalid arguemnt
(the disk) passed, which holds good for both the above cases.
What do you think ?
> Better return 0 on an unknown partition table.
>
>
> Erik
>
Suzuki
On Wed, Oct 04, 2006 at 09:50:51AM -0700, Suzuki Kp wrote:
> Erik Mouw wrote:
> >On Tue, Oct 03, 2006 at 06:00:36PM -0700, Suzuki Kp wrote:
> >
> >>Currently the rescan_partition returns 0 (success), even if it is unable
> >>to rescan the partition information ( may be due to disks offline, I/O
> >>error reading the table or unknown partition ). This would make ioctl()
> >>calls succeed for BLKRRPART requests even if partitions were not scanned
> >>properly, which is not a good thing to do.
> >>
> >>Attached here is patch to fix the issue. The patch makes
> >>rescan_partition to return -EINVAL for unknown partitions and -EIO for
> >>disk I/O errors ( or when disks are offline ).
> >
> >I don't think it's a good idea to return an error when there's an
> >unknown partition table. How do you differentiate between a device that
> >isn't partitioned at all and a device with an unknown partition table?
>
> Returning -EINVAL in both the above cases would inform the user that,
> the partition table was not read properly due to an invalid arguemnt
> (the disk) passed, which holds good for both the above cases.
>
> What do you think ?
I disagree. It's perfectly valid for a disk not to have a partition
table (for example: components of a RAID5 MD device) and we shouldn't
scare users about that. Also an unrecognised partition table format
(DEC VMS, Novell Netware, etc.) is not a reason to throw an error, it's
just unrecognised and as far as the kernel knows it's unpartioned.
It's OK to tell the user that there was some kind of device error when
scanning the partition table but IMHO an unpartitioned disk is not a
device error.
Erik
--
+-- Erik Mouw -- http://www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands
Erik Mouw wrote:
> On Wed, Oct 04, 2006 at 09:50:51AM -0700, Suzuki Kp wrote:
>
>>Erik Mouw wrote:
>>
>>>On Tue, Oct 03, 2006 at 06:00:36PM -0700, Suzuki Kp wrote:
>>>
>>>
>>>>Currently the rescan_partition returns 0 (success), even if it is unable
>>>>to rescan the partition information ( may be due to disks offline, I/O
>>>>error reading the table or unknown partition ). This would make ioctl()
>>>>calls succeed for BLKRRPART requests even if partitions were not scanned
>>>>properly, which is not a good thing to do.
>>>>
>>>>Attached here is patch to fix the issue. The patch makes
>>>>rescan_partition to return -EINVAL for unknown partitions and -EIO for
>>>>disk I/O errors ( or when disks are offline ).
>>>
>>>I don't think it's a good idea to return an error when there's an
>>>unknown partition table. How do you differentiate between a device that
>>>isn't partitioned at all and a device with an unknown partition table?
>>
>>Returning -EINVAL in both the above cases would inform the user that,
>>the partition table was not read properly due to an invalid arguemnt
>>(the disk) passed, which holds good for both the above cases.
>>
>>What do you think ?
>
>
> I disagree. It's perfectly valid for a disk not to have a partition
> table (for example: components of a RAID5 MD device) and we shouldn't
> scare users about that. Also an unrecognised partition table format
> (DEC VMS, Novell Netware, etc.) is not a reason to throw an error, it's
> just unrecognised and as far as the kernel knows it's unpartioned.
Erik,
Thank you very much for the clarification.
But don't you think that, we should let know the user that he/she is
trying to re-read a partition table from a device, which doesn't have it
or is not useful for the kernel or to him, rather than making him happy
with a success ?
>
> It's OK to tell the user that there was some kind of device error when
> scanning the partition table but IMHO an unpartitioned disk is not a
> device error.
>
>
> Erik
>
Thanks,
Suzuki
On Wed, Oct 04, 2006 at 10:37:49AM -0700, Suzuki Kp wrote:
> Erik Mouw wrote:
> >I disagree. It's perfectly valid for a disk not to have a partition
> >table (for example: components of a RAID5 MD device) and we shouldn't
> >scare users about that. Also an unrecognised partition table format
> >(DEC VMS, Novell Netware, etc.) is not a reason to throw an error, it's
> >just unrecognised and as far as the kernel knows it's unpartioned.
>
> Thank you very much for the clarification.
>
> But don't you think that, we should let know the user that he/she is
> trying to re-read a partition table from a device, which doesn't have it
> or is not useful for the kernel or to him, rather than making him happy
> with a success ?
But we already do that. This is a part of the log from a 2.6.18 kernel:
SCSI device sda: 390721968 512-byte hdwr sectors (200050 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
sda: unknown partition table
(SATA disk without partition table. It's one out of four components for
a RAID5 MD array).
Reading the partition table from an unpartitioned device *is* a
success. If that's what the user wanted, then yes, that will make him
happy.
If we let the kernel return an error like you propose, we will get a
lot of bug reports from users plugging in their new and shiny disk:
Kernel finds no partition table and throws an error. User thinks his
shiny disk is faulty, lkml tells him it's not, user then asks why the
kernel throws an error...
Erik
--
+-- Erik Mouw -- http://www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands
Erik,
Erik Mouw wrote:
> On Wed, Oct 04, 2006 at 10:37:49AM -0700, Suzuki Kp wrote:
>
>>Erik Mouw wrote:
>>
>>>I disagree. It's perfectly valid for a disk not to have a partition
>>>table (for example: components of a RAID5 MD device) and we shouldn't
>>>scare users about that. Also an unrecognised partition table format
>>>(DEC VMS, Novell Netware, etc.) is not a reason to throw an error, it's
>>>just unrecognised and as far as the kernel knows it's unpartioned.
>>
[...]
Thank you very much for the inputs.
As per the discussion I have made the changes to the patch.
This change needs to be implemented in some of the partition checkers
which doesn't do that already.
Btw, do you think it is a good idea to let the other partition checkers
run, even if one of them has failed ?
Right now, the check_partition runs the partition checkers in a
sequential manner, until it finds a success or an error.
Comments ?
Thanks,
Suzuki
On Thu, 05 Oct 2006 13:32:34 -0700
Suzuki Kp <[email protected]> wrote:
> Erik,
>
>
> Erik Mouw wrote:
> > On Wed, Oct 04, 2006 at 10:37:49AM -0700, Suzuki Kp wrote:
> >
> >>Erik Mouw wrote:
> >>
> >>>I disagree. It's perfectly valid for a disk not to have a partition
> >>>table (for example: components of a RAID5 MD device) and we shouldn't
> >>>scare users about that. Also an unrecognised partition table format
> >>>(DEC VMS, Novell Netware, etc.) is not a reason to throw an error, it's
> >>>just unrecognised and as far as the kernel knows it's unpartioned.
> >>
>
> [...]
>
>
> Thank you very much for the inputs.
>
> As per the discussion I have made the changes to the patch.
>
> This change needs to be implemented in some of the partition checkers
> which doesn't do that already.
>
> Btw, do you think it is a good idea to let the other partition checkers
> run, even if one of them has failed ?
>
> Right now, the check_partition runs the partition checkers in a
> sequential manner, until it finds a success or an error.
This is all important information to capture in the patch changelog: it
covers user-visible changes, it covers user-affecting problems with the
present kernel, it describes the implications of making this change to the
kernel, etc. All important stuff. So could you please send a complete
changelog for this patch?
>
> * Fix rescan_partition to propagate the low level I/O error.
>
Not enough ;)
>
>
> Signed Off by: Suzuki K P <[email protected]>
>
Please use "Signed-off-by:"
This patch had tabs replaced with spaces, despite the fact that it was an
attachment - that's a new one. Please get that fixed up for future
patches, thanks.
On Thu, Oct 05, 2006 at 01:32:34PM -0700, Suzuki Kp wrote:
> Btw, do you think it is a good idea to let the other partition checkers
> run, even if one of them has failed ?
Yes, just let them run. Partition information doesn't need to be on the
very first sector of the drive. If the first sector is bad and the
partition table for your funky XYZ partition table format lives on the
tenth sector, then a checker that checks the first sector would fail
and prevent your checker from running.
OTOH: having ten partition checkers check the same bad first sector
doesn't really speed up the partion check process (for that reason we
disable partition checking for drives we get for recovery). A way to
solve that would be to keep a list of bad sectors: if the first checker
finds a bad sector, it notes it down in the list so the next checker
wouldn't have to try to read that particular sector. Maybe that's too
much work to do in kernel and we'd better move the partition checking
to userland.
> Right now, the check_partition runs the partition checkers in a
> sequential manner, until it finds a success or an error.
I think it's best not to change the current behaviour and let all
partition checkers run, even if one of them failed due to device
errors. I wouldn't mind if the behaviour changed like you propose,
though.
Erik
--
+-- Erik Mouw -- http://www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands
Erik Mouw wrote:
> On Thu, Oct 05, 2006 at 01:32:34PM -0700, Suzuki Kp wrote:
>
>>Btw, do you think it is a good idea to let the other partition checkers
>>run, even if one of them has failed ?
>
>
> Yes, just let them run. Partition information doesn't need to be on the
> very first sector of the drive. If the first sector is bad and the
> partition table for your funky XYZ partition table format lives on the
> tenth sector, then a checker that checks the first sector would fail
> and prevent your checker from running.
>
> OTOH: having ten partition checkers check the same bad first sector
> doesn't really speed up the partion check process (for that reason we
> disable partition checking for drives we get for recovery). A way to
> solve that would be to keep a list of bad sectors: if the first checker
> finds a bad sector, it notes it down in the list so the next checker
> wouldn't have to try to read that particular sector. Maybe that's too
> much work to do in kernel and we'd better move the partition checking
> to userland.
>
>
>>Right now, the check_partition runs the partition checkers in a
>>sequential manner, until it finds a success or an error.
>
>
> I think it's best not to change the current behaviour and let all
> partition checkers run, even if one of them failed due to device
> errors. I wouldn't mind if the behaviour changed like you propose,
> though.
>
At present, the partition checkers doesn't run, if one of the preceeding
checker has reported an error ! *But*, some of the checkers doesn't
report the I/O error which they came across! So, this may let others
run. Thats not we want, right. We would like them to return I/O errors,
and and the check_partition should let other partition checkers continue.
Comments ?
Thanks,
Suzuki
>
> Erik
>
On Fri, Oct 06, 2006 at 10:43:42AM -0700, Suzuki K P wrote:
> Erik Mouw wrote:
> >I think it's best not to change the current behaviour and let all
> >partition checkers run, even if one of them failed due to device
> >errors. I wouldn't mind if the behaviour changed like you propose,
> >though.
> >
> At present, the partition checkers doesn't run, if one of the preceeding
> checker has reported an error ! *But*, some of the checkers doesn't
> report the I/O error which they came across! So, this may let others
> run. Thats not we want, right. We would like them to return I/O errors,
> and and the check_partition should let other partition checkers continue.
Indeed, we want them to behave the same. I.e.: a partition checker
should tell when it encounters an I/O error.
Erik
--
+-- Erik Mouw -- http://www.harddisk-recovery.nl -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands
Hi,
Here is a followup patch, which cleans up the check_partition() as well
as some of the partition check routines.
check_partition would now record the I/O error notice and proceed with
the other partitions. The error will only be reported when all of the
partitions have left the disk as "Unknown".
Cleanup for partition checkers for ibm, atari and amiga.
Comments ?
Thanks
Suzuki
Erik Mouw wrote:
> On Fri, Oct 06, 2006 at 10:43:42AM -0700, Suzuki K P wrote:
>
>>Erik Mouw wrote:
>>
>>>I think it's best not to change the current behaviour and let all
>>>partition checkers run, even if one of them failed due to device
>>>errors. I wouldn't mind if the behaviour changed like you propose,
>>>though.
>>>
>>
>>At present, the partition checkers doesn't run, if one of the preceeding
>>checker has reported an error ! *But*, some of the checkers doesn't
>>report the I/O error which they came across! So, this may let others
>>run. Thats not we want, right. We would like them to return I/O errors,
>>and and the check_partition should let other partition checkers continue.
>
>
> Indeed, we want them to behave the same. I.e.: a partition checker
> should tell when it encounters an I/O error.
>
>
> Erik
>
On Fri, 06 Oct 2006 18:46:18 -0700
Suzuki K P <[email protected]> wrote:
> * The check_partition() stops its probe once it hits an I/O error from the partition checkers. This would prevent the actual partition checker getting a chance to verify the partition. So, this patch lets the check_partition continue probing untill it hits a success while recording the I/O error which might have been reported by the checking routines.
>
> Also, it does some cleanup of the partition methods for ibm, atari and amiga to return -1 upon hitting an I/O error.
What is the significance of a `return 1', a `return 0' and a `return -EFOO' in these
functions? It all looks rather odd.