[parted-devel] Bug: Removal of BLKPG causes regression of ability to manipulate disks with other partitions in use

[Petr Uzel]

On Mon, Mar 29, 2010 at 11:01:32AM -0400, Phillip Susi wrote:
> On 3/29/2010 10:16 AM, Petr Uzel wrote:
> > This would reintroduce problems described in this thread:
> > http://lists.alioth.debian.org/pipermail/parted-devel/2010-January/003355.html
> 
> I'm not sure I understand this problem.  Transient opens from
> udev/hal/whatever should only happen right after we change the partition
> table, so this should only ever be a problem if we try to change it,
> then immediately alter it again.

Imagine you add partitions with parted in a script. Then the ioctl()s
are:

#create 1st partition
BLKPG_ADD_PART (sda1)  [A]

#create 2nd partition
BLKPG_DEL_PART (sda1)  [B]
BLKPG_ADD_PART (sda1)
BLKPG_ADD_PART (sda2)

[A] triggers udev, which runs hal, which asynchronously opens
/dev/sda1
[B] sda1 is still opened by hal -> EBUSY

And there is a good chance that 0.5s after [B], the hal will release
the device and BLKPG would succeed.

I know this happens in 'real life'.

> It looks like Ubuntu has a patch to invoke udevadm settle.  

AFAIK udevadm settle only blocks until udev process all pending
events, but it does not care about actions triggered by udev
asynchronously, like running hal.

> Perhaps this
> is another way of solving that problem?  If this is a good solution then
> maybe it should be applied upstream?
> 
> > I'm not sure the solution is possible, but I'd suggest to do something
> > like the following:
> > - if the BLKPG_DEL_PART ioctl returns EBUSY, then
> >   - check whether the partition is busy (linux_partition_is_busy() )
> >   - if it is not busy, then wait for some specific time and retry the
> >     BLKPG_DEL_PART ioctl()
> 
> If we got to this point then linux_partition_is_busy() already checked
> and thought that the partition was not mounted.  If it does, then parted
> denies the request to remove the partition  before ever trying to sync
> the partition table.

I don't think so. If I'm not mistaken, it is checked only if parted is
explicitly instructed to remove given partition, but not if the
partition is added/removed in _disk_sync_part_table().

> If it actually turns out that it is busy, then
> perhaps a 1 second sleep followed by a retry might help,

Sleep and wait after every EBUSY is IMHO not a good idea, because
then modifications of partition table on disks with many mounted
partitions would take a long time -> for this reason I suggest to
wait only if the partition is not mounted.

> though again, I
> don't see why this would happen unless we sync the partition table twice
> in rapid succession without a call to udevadm settle.

As I wrote above, udevadm settle is not enough to prevent the race
with hal.

> 
> >> +				memset( &geom, 0, sizeof( geom ) );
> >> +				length = 0;
> >> +				/* get start and length of existing partition */
> >> +				dev_name = _device_get_part_path (disk->dev, i);
> >> +				fd = open( dev_name, O_RDONLY );
> >> +				ioctl( fd, HDIO_GETGEO, &geom );
> >> +				ioctl( fd, BLKGETSIZE64, &length );
> >> +				length /= disk->dev->sector_size;
> > 
> > Why not use _device_get_length() here?
> 
> I suppose I could.  Looking at that it looks like I could also use
> LINUX_SPECIFIC(dev)->fd instead of building the name and calling open()
> myself.
> 
> >> +				close( fd );
> > 
> > I think there should be some checks of return values - if it
> > is not possible to open the device for whatever reason, or issue the
> > ioctl()s, then the code should jump directly to ped_exception_throw().
> 
> I didn't bother checking since I initialize the size and geometry to
> zero, so if any of the open or ioctl calls fail, they will still be
> zero, which will not match the current start and length, so we throw the
> warning anyhow.

If you are sure that failed ioctl does not alter the third argument
(I'm not BTW), then OK. On the other hand, short comment that error
checking is not necessary wouldn't hurt :) Sorry if this is
nitpicking.

Petr

--
Petr Uzel, openSUSE Boosters Team
IRC: ptr_uzl @ freenode
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20100329/0f587fdb/attachment.pgp>