bug#15299: [PATCH 1/9] parted: fix EOF and ctrl-c handling

[Brian C. Lane]

On Sat, Sep 07, 2013 at 12:29:55PM -0400, Phillip Susi wrote:
> Hi Brian, it's been a while and I still haven't heard back from you.
> Could you provide some more comments so I can try to get your concerns
> addressed and get these applied?  I also have had time to follow up on
> some of these, see comments inline.
> 
> On 08/13/2013 11:46 PM, Phillip Susi wrote:
> > On 08/08/2013 08:45 PM, Brian C. Lane wrote:
> >> Commit messages need to follow the normal parted form. I'd also
> >> like to see tests as separate commits.
> > 
> > Can you be a bit more specific about the form?  As far as I know
> > this is the usual form that Jim had me use.  He also stressed that
> > new feature and test for it go hand in hand, and logically they are
> > two parts of the same conceptual change, so it makes sense to keep
> > them in the same commit.

I generally like them to be in a separate commit so that they can be
applied by themselves so I can see it fail before applying the fix.

Also, Jim wanted to use the standard GNU commit message style of:

* filename (function): change summary

Personally I think that's overkill now that we use git instead of CVS,
but it is consistent with previous practice.

> >> 4/9 - Nak. I have a more general patch for this -- not
> >> everything inserts DMRAID in there.
> > 
> > Could you be a bit more specific?

See commit 4 from the patches I just sent, it preserves the UUID if it
exists, but doesn't make up a new one (which may break things expecting
a certain form).

> >> 5/9 - Shouldn't it use disk->dev->sector_size instead of
> >> hard-coded 512/1024? The logic seems a bit messy, using an if
> >> block instead of the == 1 ? 512 : 1024 may clean it up a little.
> >> If it is already set to 512 (or really ->sector_size) we don't
> >> need to walk the other partitions. One place where a tab snuck
> >> in. Looks pretty ok other than that.
> > 
> > I'm not sure what you mean about walking the other partitions.  I 
> > suppose it should use ->sector size.  I'll have to check the
> > kernel sources again to see how they behave with non 512 byte
> > sectors.
> 
> I checked the kernel and yes, it does round up to the sector size for
> > 512b sectors, so I'll fix this.

Cool, thanks.

> >> 6/9 - Nak. I'm not sure if this is the right way to handle
> >> things.
> > 
> > Could you be more specific?

Let me see if I understand the problem. The issue is that the disk isn't
right, the gpt backup is in the wrong location and currently we will
sorta fix that by writing to the correct location if changes are made
even if told to ignore it.

I'd lean towards not allowing any operations at all until the location
is fixed. If it isn't at the end it is in the wrong place and we really
have no way to tell why it isn't right.

I really don't like the idea of operating on the backup when it isn't in
the right location. I'd rather give the user 2 options:

1. fix the backup location
2. quit

Unrelated to that, I also notice that the gpt-head-move.py script isn't
taking into account endianness. I just ran into a similar problem with
the gpt-header-munge script. You can't just pack/unpack from the header
without converting from little-endian.

> >> 7/9 - Nak. I really don't like the giant pile of changes for a
> >> problem that is only in the gpt code.
> > 
> > Do you have another suggestion?  I don't see another way of fixing
> > the bug, and the constant re-reading of the partition table seems
> > a senseless waste of time to begin with, so it kills two birds with
> > one stone.

I don't have any better ideas yet. I'm just worried all those changes
are going to break something subtle.

> >> 8/9 - Nak. It looks to me like removing some of these checks is
> >> going to allow user (ui or library) checks that should fail on
> >> new data to pass. I'm ok with letting us run with these errors so
> >> we can fix them, but you shouldn't be able to enter bad numbers
> >> after the warnings.
> > 
> > I thought that the ui did its own checks and stops you before it
> > gets to the library but I'll take a look at that again.
> 
> I just tested it and trying to create an overlapping partition does
> still throw the message that it can't be done, this is the closest we
> can manage, is that ok?  So parted won't let you create a bad table.
> I don't have a good way of testing it but I'm pretty sure that the
> library will warn another caller about it and they will have to choose
> to ignore the exception to proceed with creating a bad table.  I think
> that's OK.

Ok, good. Thanks for looking at that.

> >> 9/9 - Nak. I need more details here. The reasoning in the
> >> original commit seems sound to me, and I'd be more worried about
> >> busy devices than the filesystem not matching (I'm not sure we
> >> should even be in the fs detection business anymore).
> > 
> > The reasoning was sound, it was just based on a falsehood: the
> > kernel does indeed still maintain two separate caches for the two
> > devices, so when you write a fs superblock to the partition device
> > then try to read it via the whole disk device, you don't see the
> > superblock unless you flush the caches.  See:
> > 
> > http://lists.alioth.debian.org/pipermail/parted-devel/2012-December/004289.html
> > 
> > I never heard back from Hans about why he put that in, but removing
> > it fixed the problem, and I couldn't find any attempt to resolve
> > the cache aliasing problem in the code, or heard of anyone talking
> > about it on linux-mm.

Ends up I just had to do something similar to get filesystem probing
working all the time. See patch 18 in my set. What I did was keep the
kernel 2.6 check and skip the sync on the individual partitions. I think
Hans' point about udev, etc. churn while doing this is a good one.
parted only operates on offsets inside the parent device anyway so I
think it is better to only sync that.

Also note that it only seems to happen on bare metal. We weren't able to
reproduce the problems in a KVM.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)

pgpEI6AsyeL1g.pgp
Description: PGP signature