All of lore.kernel.org
 help / color / mirror / Atom feed
* sfdisk, re-eading partition table fails
@ 2015-12-14 12:11 Ruediger Meier
  2015-12-15 10:24 ` Karel Zak
  0 siblings, 1 reply; 5+ messages in thread
From: Ruediger Meier @ 2015-12-14 12:11 UTC (permalink / raw)
  To: util-linux

Hi,

our test suite shows that many sfdisk tests fail sometimes to "Re-read
partition table" at the end. I was able to find some systems where
I could re-produce the problem using the script below.

Seems that the problem is because of the first BLKRRPART ioctl call
in sfdisk.c function is_device_used(). Maybe it cause udev or
whatever to open the device and then the real BLKRRPART in
write_changes() fails.

Removing the first BLKRRPART ioctl (or sleeping about 50ms after the first one)
"fixes" the issue.

------ my test script -------------
#!/bin/bash
export LANG="C"
export LC_ALL="C"

export LD_LIBRARY_PATH=/home/rudi/devel/util-linux/build/.libs
SFDISK=/home/rudi/devel/util-linux/build/.libs/sfdisk
LOG=/tmp/sfdisk.log

modprobe -r scsi_debug || exit 1
modprobe -b scsi_debug dev_size_mb=100 sector_size=512 || exit 1
udevadm settle
sleep 2
DEVICE=/dev/$(grep --with-filename scsi_debug /sys/block/*/device/model | awk -F '/' '{print $4}')
echo device: $DEVICE

rm -f "$LOG"

# create a partition
echo ',+,L' | $SFDISK --label dos ${DEVICE} &> /dev/null
udevadm settle
sleep 2

echo "+10M,-10M" | strace -f -o /tmp/strace $SFDISK -N1 ${DEVICE} >> "$LOG" 2>&1
if grep -qi "failed" "$LOG"; then 
	echo "!! FAILED !!"
	exit 1
fi
exit 0
----------

cu,
Rudi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: sfdisk, re-eading partition table fails
  2015-12-14 12:11 sfdisk, re-eading partition table fails Ruediger Meier
@ 2015-12-15 10:24 ` Karel Zak
  2015-12-15 12:26   ` Ruediger Meier
  0 siblings, 1 reply; 5+ messages in thread
From: Karel Zak @ 2015-12-15 10:24 UTC (permalink / raw)
  To: Ruediger Meier; +Cc: util-linux

On Mon, Dec 14, 2015 at 01:11:59PM +0100, Ruediger Meier wrote:
> our test suite shows that many sfdisk tests fail sometimes to "Re-read
> partition table" at the end. I was able to find some systems where
> I could re-produce the problem using the script below.

Yes, the problem is probably udev, and "udevadm settle" is not always
enough.

> Seems that the problem is because of the first BLKRRPART ioctl call
> in sfdisk.c function is_device_used(). Maybe it cause udev or
> whatever to open the device and then the real BLKRRPART in
> write_changes() fails.

but the device has to be already partitioned, on disk without
partitions BLKRRPART (and is_device_used()) does not generate any
events (try "udevadm monitor").

> Removing the first BLKRRPART ioctl (or sleeping about 50ms after the first one)
> "fixes" the issue.

The is_device_used() in the sfdisk is nothing elegant, maybe
we can use --noreread sfdisk command line option in the tests.

Anyway, it would be nice to have some in-sfdisk solution, because our
users who use fdisk in scripts may be affected by the same problem.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: sfdisk, re-eading partition table fails
  2015-12-15 10:24 ` Karel Zak
@ 2015-12-15 12:26   ` Ruediger Meier
  2015-12-15 13:33     ` Karel Zak
  2015-12-18 14:32     ` Ruediger Meier
  0 siblings, 2 replies; 5+ messages in thread
From: Ruediger Meier @ 2015-12-15 12:26 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux

On Tuesday 15 December 2015, Karel Zak wrote:
> On Mon, Dec 14, 2015 at 01:11:59PM +0100, Ruediger Meier wrote:
> > our test suite shows that many sfdisk tests fail sometimes to
> > "Re-read partition table" at the end. I was able to find some
> > systems where I could re-produce the problem using the script
> > below.
>
> Yes, the problem is probably udev, and "udevadm settle" is not always
> enough.

I don't see any problem with "udevadm settle" between our sfdisk
commands. Looks like we would need "udevadm settle" within sfdisk.

> > Seems that the problem is because of the first BLKRRPART ioctl call
> > in sfdisk.c function is_device_used(). Maybe it cause udev or
> > whatever to open the device and then the real BLKRRPART in
> > write_changes() fails.
>
> but the device has to be already partitioned, on disk without
> partitions BLKRRPART (and is_device_used()) does not generate any
> events (try "udevadm monitor").

Yes I had checked that. This explains why only the resize/move/etc
tests fail.

> > Removing the first BLKRRPART ioctl (or sleeping about 50ms after
> > the first one) "fixes" the issue.
>
> The is_device_used() in the sfdisk is nothing elegant,

But how else could we check whether a device is in use?

> maybe 
> we can use --noreread sfdisk command line option in the tests.

I have such patch already for testing.

> Anyway, it would be nice to have some in-sfdisk solution, because our
> users who use fdisk in scripts may be affected by the same problem.

Yes. Now since I know about these problems I find the default case
without --no-reread a bit dangerous. After using sfdisk one should
always check if re-reading was successful before doing things with an
outdated partition table. Some weeks ago I've made such a mistake and
it was real disaster.

An option like --return-error-if-re-read-fails could be useful. Or do
we have a command with tells us whether kernel partition table
matches the disk? 

BTW what happend to
 -R, --re-read     make the kernel reread the partition table
?

Also IMO interesting, this commit message from GNU parted:
------------- 
commit 1223b9fc07859cb619c80dc057bd05458f9b5669
Author: Jim Meyering <meyering@redhat.com>
Date:   Fri Apr 30 11:45:51 2010 +0200

    libparted: remove now-worse-than-useless _kernel_reread_part_table

    Now that we're using BLKPG properly, there's no point in using the
    less-functional BLKRRPART ioctl to make the kernel reread the partition
    table.
    More importantly, this function would fail when any partition is in
    use, in spite of our having carefully vetted them via BLKPG ioctls.
    * libparted/arch/linux.c (_kernel_reread_part_table): Remove function.
    (linux_disk_commit): Don't call it.
-------------

I think partprobe can re-read a partition table partly (if not all affected
partitions are in use.)


cu,
Rudi



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: sfdisk, re-eading partition table fails
  2015-12-15 12:26   ` Ruediger Meier
@ 2015-12-15 13:33     ` Karel Zak
  2015-12-18 14:32     ` Ruediger Meier
  1 sibling, 0 replies; 5+ messages in thread
From: Karel Zak @ 2015-12-15 13:33 UTC (permalink / raw)
  To: Ruediger Meier; +Cc: util-linux

On Tue, Dec 15, 2015 at 01:26:33PM +0100, Ruediger Meier wrote:
> > > Seems that the problem is because of the first BLKRRPART ioctl call
> > > in sfdisk.c function is_device_used(). Maybe it cause udev or
> > > whatever to open the device and then the real BLKRRPART in
> > > write_changes() fails.
> >
> > but the device has to be already partitioned, on disk without
> > partitions BLKRRPART (and is_device_used()) does not generate any
> > events (try "udevadm monitor").
> 
> Yes I had checked that. This explains why only the resize/move/etc
> tests fail.
> 
> > > Removing the first BLKRRPART ioctl (or sleeping about 50ms after
> > > the first one) "fixes" the issue.
> >
> > The is_device_used() in the sfdisk is nothing elegant,
> 
> But how else could we check whether a device is in use?

maybe open(O_EXCL), but it works for the device only. The BLKRRPART
ioctl fails when any partition is also in use (mounted or so).

And note that the current code in sfdisk does not provide any
exclusive access to the disk, it just verifies that the disk is not
used in time you call BLKRRPART.

For fdisk and sfdisk we do not check if device is in use.

We already use libudev in util-liux, maybe add "udevadm monitor"
functionality to sfdisk is not so bad idea, or (ideally) we need 
something less aggressive than BLKRRPART.

> > maybe 
> > we can use --noreread sfdisk command line option in the tests.
> 
> I have such patch already for testing.
> 
> > Anyway, it would be nice to have some in-sfdisk solution, because our
> > users who use fdisk in scripts may be affected by the same problem.
>
> Yes. Now since I know about these problems I find the default case
> without --no-reread a bit dangerous. After using sfdisk one should
> always check if re-reading was successful before doing things with an
> outdated partition table. Some weeks ago I've made such a mistake and
> it was real disaster.
> 
> An option like --return-error-if-re-read-fails could be useful. Or do
> we have a command with tells us whether kernel partition table
> matches the disk? 

This is interesting idea, it would be nice to have command to compare
stuff from /sys ("size" and "start" device attributes) with on-disk
partition table. Not sure if something like this belongs to fdisk-like 
program, maybe "partx" would be better for this functionality.

> BTW what happend to
>  -R, --re-read     make the kernel reread the partition table
> ?

 we have "blockdev --rereadpt"

> Also IMO interesting, this commit message from GNU parted:
> ------------- 
> commit 1223b9fc07859cb619c80dc057bd05458f9b5669
> Author: Jim Meyering <meyering@redhat.com>
> Date:   Fri Apr 30 11:45:51 2010 +0200
> 
>     libparted: remove now-worse-than-useless _kernel_reread_part_table
> 
>     Now that we're using BLKPG properly, there's no point in using the
>     less-functional BLKRRPART ioctl to make the kernel reread the partition
>     table.
>     More importantly, this function would fail when any partition is in
>     use, in spite of our having carefully vetted them via BLKPG ioctls.
>     * libparted/arch/linux.c (_kernel_reread_part_table): Remove function.
>     (linux_disk_commit): Don't call it.
> -------------
> 
> I think partprobe can re-read a partition table partly (if not all affected
> partitions are in use.)

Yes, parted tries to be smart and it updates only affected partitions.

I guess there is still many situations when it's good idea to call
BLKRRPART, for example if you change in-PT stored partition attributes
(UUID, flags, etc.), reorder partitions, etc. The BLKPG ioctls has
been introduced to support partitions add/del/resize.

But I think BLKRRPART-after-write is not so big problem for sfdisk.
The problem is the initial BLKRRPART.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: sfdisk, re-eading partition table fails
  2015-12-15 12:26   ` Ruediger Meier
  2015-12-15 13:33     ` Karel Zak
@ 2015-12-18 14:32     ` Ruediger Meier
  1 sibling, 0 replies; 5+ messages in thread
From: Ruediger Meier @ 2015-12-18 14:32 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux

On Tuesday 15 December 2015, Ruediger Meier wrote:
> On Tuesday 15 December 2015, Karel Zak wrote:
> > On Mon, Dec 14, 2015 at 01:11:59PM +0100, Ruediger Meier wrote:

> > > Removing the first BLKRRPART ioctl (or sleeping about 50ms after
> > > the first one) "fixes" the issue.
> >
> > The is_device_used() in the sfdisk is nothing elegant,
>
> But how else could we check whether a device is in use?
>
> > maybe
> > we can use --noreread sfdisk command line option in the tests.
>
> I have such patch already for testing.

It's now on github pull request #244 among other patches
https://github.com/karelzak/util-linux/pull/244

Would be still nice to fix the problem within sfdisk somehow.

BTW I found that is_device_used() is not called for other generic 
options like --reorder. This seems inconsistent.

cu,
Rudi

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-12-18 14:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-14 12:11 sfdisk, re-eading partition table fails Ruediger Meier
2015-12-15 10:24 ` Karel Zak
2015-12-15 12:26   ` Ruediger Meier
2015-12-15 13:33     ` Karel Zak
2015-12-18 14:32     ` Ruediger Meier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.