lkml.org 
[lkml]   [2011]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] Fix over-zealous flush_disk when changing device size.
From
Date
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
> On Fri, 04 Mar 2011 10:25:06 -0700 Andrew Patterson <andrew.patterson@hp.com>
> wrote:
>
> > On Fri, 2011-03-04 at 11:16 +1100, NeilBrown wrote:
> > > On Thu, 3 Mar 2011 09:31:20 -0500 Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > > On Thu, Feb 17, 2011 at 04:50:57PM +1100, NeilBrown wrote:
> > > > >
> > > > > Hi Andrew (and others)
> > > > > I wonder if you would review the following for me and comment.
> > > >
> > > > Please send think in this area through -fsdevel next time, thanks!
> > >
> > > Will try to remember - it is sometimes hard to get this sort of patch before
> > > the right audience ... I thought "block layer" rather than "file systems" :-(
> > >
> > > Thanks for finding it anyway.
> > >
> > > >
> > > > > There are two cases when we call flush_disk.
> > > > > In one, the device has disappeared (check_disk_change) so any
> > > > > data will hold becomes irrelevant.
> > > > > In the oter, the device has changed size (check_disk_size_change)
> > > > > so data we hold may be irrelevant.
> > > > >
> > > > > In both cases it makes sense to discard any 'clean' buffers,
> > > > > so they will be read back from the device if needed.
> > > >
> > > > Does it? If the device has disappeared we can't read them back anyway.
> > >
> > > I think that is the point - return an error rather than stale data.
> > >
> > > > If the device has resized to a smaller size the same is true about
> > > > those buffers that have gone away, and if it has resized to a larger
> > > > size invalidating anything doesn't make sense at all. I think this
> > > > area needs more love than a quick kill_dirty hackjob.
> > >
> > > I tend to agree. I wasn't entirely convinced by the changelog comments on
> > > the original offending patch, but I couldn't convince myself there was no
> > > justification either, and I wanted to fix the corruption I saw - while close
> > > to the end of a release cycle - without introducing any new regressions.
> > >
> > > >
> > > > > In the former case it makes sense to discard 'dirty' buffers
> > > > > as there will never be anywhere safe to write the data. In the
> > > > > second case it *does*not* make sense to discard dirty buffers
> > > > > as that will lead to file system corruption when you simply enlarge
> > > > > the containing devices.
> > > >
> > > > Doing anything like this at the buffer cache layer or inode cache layer
> > > > doesn't make any sense. If a device goes away or shrinks below the
> > > > filesystem size the filesystem simply needs to be shut down and in te
> > > > former size the admin needs to start a manual repair. Trying to do
> > > > any botch jobs in lower layer never works in practice.
> > >
> > > Amen.
> > > What I personally would really like to see is an interface for the block
> > > device to say to the filesystem (or more specifically: whatever has bdclaimed
> > > it) "I am about to resize to $X - is that OK?" and also "I have resized -
> > > deal with it".
> > >
> > > >
> > > > For now I think the best short term fix is to simply revert commit
> > > > 608aeef17a91747d6303de4df5e2c2e6899a95e8
> > > >
> > > > "Call flush_disk() after detecting an online resize."
> > >
> > > You may be right, but I suspect that Andrew Patterson had a real issue to
> > > solve which lead to submitting it, and I'd really like to understand that
> > > issue before I would feel confident just reverting it.
> > >
> > > Andrew: are you out there? Can you provide some background for your patch?
> >
> > I put in the flush disk stuff at the suggestion of James Bottomley. In
> > fact the text for the justification in 608aeef17a91747d6303 is mostly
> > his. The idea is to get errors reported immediately rather than waiting
> > around for them to eventually get flushed and to make sure stale data is
> > not kept around. Certainly, at a minimum, not keeping stale data around
> > seems valuable to me.
> >
> > What parts of the original justification did you think are unconvincing?
> > Note that the flush for growing the device is really only there for the
> > degenerate case where someone might shrink then grow a device
> > (admittedly, the user probably deserves to get data corruption/security
> > holes in such a case).
>
> hi Andrew.
>
> One of the things that I didn't like about the change log is that it didn't
> give clear context - what exactly is the problem it is trying to fix?

Well true, but the rest of the patch set might have given the context.

> When you talk about "disks" changing size (reduced radius:-?) I think first
> of 'dm' and 'md' - yet it clearly isn't dm related as dm doesn't even use
> that code, and if it was 'md' related I would have thought I would have
> heard about it....
> So presumably these are some SCSI-attached devices that do internal volume
> management and can change the size of .... targets? LUNs? something like
> that.

Yes. The idea is that you want to increase/decrease the size of the
underlying block device (usually a FC or iSCSI LUN). Short of a reboot,
the OS will not detect the size changed. I added some code that rescans
the LUN and gets the new size and report the new size to the block
layer.

> So can these things change size without the SCSI layer immediately knowing
> about it?? Don't you get some sort of "unit attention" or something?
>

In some cases with some hardware yes. In most, no.

> The idea that the device might reduce in size and then grow again seems just
> plain wrong.

This would usually be due to some user error. The user may have shrunk
the device too much, realized there error and then corrected it. I admit
that it is pretty far-fetched.

> If that is possible it could return to it's original size and
> you would never notice?

Yes.

> And if there are cases that you know you will never
> notice, then it seems like it is the wrong solution.
> It would have been more credible if it *only* tried to flush when the size
> was reduced ... which you do suggest as a possibility above I think.
>
>
> The "potential security hole" seems completely bogus.
> If you give me permission to read something, and I do, then you remove that
> permission, the fact that I still remember what I read is not a security
> hole. It is a natural fact of life

I think the case here might be where you add a LUN onto the space where
the previous LUN space existed. Again, not likely.

>
> As for the two cases: I would describe them:
> 1. planned. The fs is already shrunk to within the new boundary so there
> is nothing to be gained by flushing, and we could have to reload a
> pile of data from storage which would be pointless

This is the security case.

> 2. unplanned. The fs is probably toast, so whether we invalidate or not
> isn't going to make a whole lot of difference.

Agreed.

>
> So I would vote for not flushing.
>
> The "not keeping stale data around" argument ... the only data that is
> clearly stale is data that is in the block-device cache and beyond the new
> EOF. Most filesystems keep most data in the page cache rather than the
> block device cache so this would just be some metadata - maybe inode tables
> or block usage bitmaps or similar. And caches regularly keep data around
> that isn't actually going to be used - so keeping a bit more on the rare
> occasion of a block-device-resize doesn't seem like an important cost.
>
> Getting errors a little bit earlier in the case of an unplanned shrink is
> possibly a credible argument. This would be read errors only of course -
> write errors would still arrive at the same time. I'm not sure it would
> really be very much earlier...maybe a bit in some cases.
>
> On the whole, the arguments both for and against this change - in principle
> - seem rather weak: "maybe" and "possibly" rather than something concrete.
>
> So given that (due to an oversight) it actually causes filesystem
> corruption, I tend to agree with Christoph that the best approach at this
> stage is the revert the original patch, and then review all the related code
> and come up with a "correct" approach.
>
>
> And just to clarify: when I first found that description "unconvincing" I
> hadn't thought through all of these issues - I think it was just that the
> justification seems vague rather than concrete, so it was hard to reason
> about it.
>
>
> Would you be uncomfortable if I asked Linus to revert both my fix and your
> original patch??

James Bottomley wanted me to put this functionality in. I have no
problem with reverting it myself, especially if it is causing other
problems. I would have to say that you need to ask him (or rather, I am
not qualified to render an opinion here).

Andrew




\
 
 \ /
  Last update: 2011-03-07 05:37    [W:0.242 / U:2.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site