| Message ID | Pine.LNX.4.64.0907270913170.25437@hs20-bc2-1.build.redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alasdair Kergon |
| Headers | show |
On Mon, Jul 27 2009 at 9:21am -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > Hi > > This patch fixes a bug introduced in 2.6.31-rc1. Targets that define > iterate_devices and don't contain any devices (such as dm-loop in fsio) > don't work. > > Mikulas > > --- > > Fix reverse logic in device_area_is_valid > > ->iterate_devices method calls the callback for every underlying device > in the target. If the callback returns non-zero, iterate_devices exits > and returns this value. If the callback returns zero for all the devices, > iterate_devices returns zero. > > The logic to check for invalid device areas was reversed. > device_area_is_valid returned 0 on error and 1 on success. > > Thus: > - error was returned only if all the devices vere errorneous. If some of them > returned 1 (success), dm_calculate_queue_limits understood this as success. > - if the target had no device (the example is dm-loop target in FSIO mode), > iterate_devices returned error straight away and dm_calculate_queue_limits > understood this as an error. > > This patch reverses the logic so that 0 means success and 1 means error. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Nice catch, interesting that this was somehow missed. All targets do treat a non-zero return from .iterate_devices as an error. We should probably rename device_area_is_valid to device_area_invalid. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 27 Jul 2009, Mike Snitzer wrote: > On Mon, Jul 27 2009 at 9:21am -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > Hi > > > > This patch fixes a bug introduced in 2.6.31-rc1. Targets that define > > iterate_devices and don't contain any devices (such as dm-loop in fsio) > > don't work. > > > > Mikulas > > > > --- > > > > Fix reverse logic in device_area_is_valid > > > > ->iterate_devices method calls the callback for every underlying device > > in the target. If the callback returns non-zero, iterate_devices exits > > and returns this value. If the callback returns zero for all the devices, > > iterate_devices returns zero. > > > > The logic to check for invalid device areas was reversed. > > device_area_is_valid returned 0 on error and 1 on success. > > > > Thus: > > - error was returned only if all the devices vere errorneous. If some of them > > returned 1 (success), dm_calculate_queue_limits understood this as success. > > - if the target had no device (the example is dm-loop target in FSIO mode), > > iterate_devices returned error straight away and dm_calculate_queue_limits > > understood this as an error. > > > > This patch reverses the logic so that 0 means success and 1 means error. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Nice catch, interesting that this was somehow missed. All targets do > treat a non-zero return from .iterate_devices as an error. > > We should probably rename device_area_is_valid to device_area_invalid. > > Mike Yes, you can rename it to that. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux-2.6.31-rc3-devel/drivers/md/dm-table.c =================================================================== --- linux-2.6.31-rc3-devel.orig/drivers/md/dm-table.c 2009-07-26 22:11:53.000000000 +0200 +++ linux-2.6.31-rc3-devel/drivers/md/dm-table.c 2009-07-26 22:12:39.000000000 +0200 @@ -357,16 +357,16 @@ static int device_area_is_valid(struct d char b[BDEVNAME_SIZE]; if (!dev_size) - return 1; + return 0; if ((start >= dev_size) || (start + ti->len > dev_size)) { DMWARN("%s: %s too small for target", dm_device_name(ti->table->md), bdevname(bdev, b)); - return 0; + return 1; } if (logical_block_size_sectors <= 1) - return 1; + return 0; if (start & (logical_block_size_sectors - 1)) { DMWARN("%s: start=%llu not aligned to h/w " @@ -374,7 +374,7 @@ static int device_area_is_valid(struct d dm_device_name(ti->table->md), (unsigned long long)start, limits->logical_block_size, bdevname(bdev, b)); - return 0; + return 1; } if (ti->len & (logical_block_size_sectors - 1)) { @@ -383,10 +383,10 @@ static int device_area_is_valid(struct d dm_device_name(ti->table->md), (unsigned long long)ti->len, limits->logical_block_size, bdevname(bdev, b)); - return 0; + return 1; } - return 1; + return 0; } /* @@ -1005,8 +1005,8 @@ int dm_calculate_queue_limits(struct dm_ * Check each device area is consistent with the target's * overall queue limits. */ - if (!ti->type->iterate_devices(ti, device_area_is_valid, - &ti_limits)) + if (ti->type->iterate_devices(ti, device_area_is_valid, + &ti_limits)) return -EINVAL; combine_limits:
Hi This patch fixes a bug introduced in 2.6.31-rc1. Targets that define iterate_devices and don't contain any devices (such as dm-loop in fsio) don't work. Mikulas --- Fix reverse logic in device_area_is_valid ->iterate_devices method calls the callback for every underlying device in the target. If the callback returns non-zero, iterate_devices exits and returns this value. If the callback returns zero for all the devices, iterate_devices returns zero. The logic to check for invalid device areas was reversed. device_area_is_valid returned 0 on error and 1 on success. Thus: - error was returned only if all the devices vere errorneous. If some of them returned 1 (success), dm_calculate_queue_limits understood this as success. - if the target had no device (the example is dm-loop target in FSIO mode), iterate_devices returned error straight away and dm_calculate_queue_limits understood this as an error. This patch reverses the logic so that 0 means success and 1 means error. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- drivers/md/dm-table.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel