Hi Andrew (and others)
I wonder if you would review the following for me and comment.
Thanks,
NeilBrown
From e7f75c2a757108cdd83ce8c808a16bf27686c95f Mon Sep 17 00:00:00 2001
From: NeilBrown <redacted>
Date: Thu, 17 Feb 2011 16:37:30 +1100
Subject: [PATCH] Fix over-zealous flush_disk when changing device size.
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.
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.
flush_disk calls __invalidate_devices.
__invalidate_device calls both invalidate_inodes and invalidate_bdev.
invalidate_inodes *does* discard I_DIRTY inodes and this does lead
to fs corruption.
invalidate_bev *does*not* discard dirty pages, but I don't really care
about that at present.
So this patch adds a flag to __invalidate_device (calling it
__invalidate_device2) to indicate whether dirty buffers should be
killed, and this is passed to invalidate_inodes which can choose to
skip dirty inodes.
flusk_disk then passes true from check_disk_change and false from
check_disk_size_change.
dm avoids tripping over this problem by calling i_size_write directly
rathher than using check_disk_size_change.
md does use check_disk_size_change and so is affected.
This regression was introduced by commit 608aeef17a
which causes check_disk_size_change to call
flush_disk.
Cc: stable@kernel.org
Cc: Andrew Patterson <redacted>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: NeilBrown <redacted>
---
fs/block_dev.c | 12 ++++++------
fs/inode.c | 6 +++++-
fs/internal.h | 2 +-
include/linux/fs.h | 6 +++++-
4 files changed, 17 insertions(+), 9 deletions(-)
From: Christoph Hellwig <hch@infradead.org> Date: 2011-03-03 14:31:25
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!
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.
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.
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.
For now I think the best short term fix is to simply revert commit
608aeef17a91747d6303de4df5e2c2e6899a95e8
"Call flush_disk() after detecting an online resize."
On Thu, 3 Mar 2011 09:31:20 -0500 Christoph Hellwig [off-list ref] wrote:
On Thu, Feb 17, 2011 at 04:50:57PM +1100, NeilBrown wrote:
quoted
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.
quoted
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.
quoted
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?
Thanks,
NeilBrown
From: Andrew Patterson <hidden> Date: 2011-03-04 17:35:10
On Fri, 2011-03-04 at 11:16 +1100, NeilBrown wrote:
On Thu, 3 Mar 2011 09:31:20 -0500 Christoph Hellwig [off-list ref] wrote:
quoted
On Thu, Feb 17, 2011 at 04:50:57PM +1100, NeilBrown wrote:
quoted
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.
quoted
quoted
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.
quoted
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.
quoted
quoted
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".
quoted
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).
Andrew
On Fri, 04 Mar 2011 10:25:06 -0700 Andrew Patterson [off-list ref]
wrote:
On Fri, 2011-03-04 at 11:16 +1100, NeilBrown wrote:
quoted
On Thu, 3 Mar 2011 09:31:20 -0500 Christoph Hellwig [off-list ref] wrote:
quoted
On Thu, Feb 17, 2011 at 04:50:57PM +1100, NeilBrown wrote:
quoted
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.
quoted
quoted
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.
quoted
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.
quoted
quoted
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".
quoted
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?
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.
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?
The idea that the device might reduce in size and then grow again seems just
plain wrong. If that is possible it could return to it's original size and
you would never notice? 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
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
2. unplanned. The fs is probably toast, so whether we invalidate or not
isn't going to make a whole lot of difference.
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??
Thanks,
NeilBrown
From: Andrew Patterson <hidden> Date: 2011-03-07 04:33:52
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
On Fri, 04 Mar 2011 10:25:06 -0700 Andrew Patterson [off-list ref]
wrote:
quoted
On Fri, 2011-03-04 at 11:16 +1100, NeilBrown wrote:
quoted
On Thu, 3 Mar 2011 09:31:20 -0500 Christoph Hellwig [off-list ref] wrote:
quoted
On Thu, Feb 17, 2011 at 04:50:57PM +1100, NeilBrown wrote:
quoted
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.
quoted
quoted
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.
quoted
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.
quoted
quoted
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".
quoted
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
From: James Bottomley <hidden> Date: 2011-03-07 16:47:05
On Sun, 2011-03-06 at 21:22 -0700, Andrew Patterson wrote:
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
quoted
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).
So it seems we have a couple of problems: the first being that
flush_disk() doesn't actually do what it says (flush the disk). If it's
just discarding all data, dirty or clean, then its use in the
grow/shrink interface is definitely wrong.
The idea is that before we complete the grow/shrink, we make sure that
the device doesn't have any errors, so we want to try to write out all
dirty buffers to make sure they still have a home. If flush_disk()
doesn't do that, then we need to use a different interface ... what's
the interface to flush a disk?
James
(linux-fsdevel added - seems relevant)
On Mon, 07 Mar 2011 10:46:58 -0600 James Bottomley [off-list ref]
wrote:
On Sun, 2011-03-06 at 21:22 -0700, Andrew Patterson wrote:
quoted
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
quoted
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).
So it seems we have a couple of problems: the first being that
flush_disk() doesn't actually do what it says (flush the disk). If it's
just discarding all data, dirty or clean, then its use in the
grow/shrink interface is definitely wrong.
The idea is that before we complete the grow/shrink, we make sure that
the device doesn't have any errors, so we want to try to write out all
dirty buffers to make sure they still have a home. If flush_disk()
doesn't do that, then we need to use a different interface ... what's
the interface to flush a disk?
Hi James,
I *always* want to make sure that my device doesn't have any errors, not just
when it changes size... but I'm not sure that regularly flushing out data is
the right way to do it.
But maybe I still misunderstand what the real point of this is.
As for the correct interface to flush a disk - there isn't one.
One doesn't flush a storage device, one flushes a cache - to a storage
device. And there is not a 1-to-1 mapping.
A block device *is* associated with one cache - one which is used for
caching accesses through /dev/XXX and also by some filesystems to cache some
metadata. You can flush this cache with sync_blockdev(). This just
flushes out dirty pages in that cache, it doesn't discard clean pages.
invalidate_bdev() can discard clean pages. Call both, and get both outcomes.
If a filesystem is mounted directly on a given block_device, then it
should have a valid s_bdev pointer and it is possible to find that filesystem
from the block_device using get_active_super(). You could then call
sync_filesystem() to flush out dirty data. There isn't really a good
interface to discard clean data. shrink_dcache_sb() does some of it, other
bits of code do other bits.
Note that a block_device also can have a pointer to a super_block (bd_super).
This does not seem to be widely used .... ext3 and ext4 use it so that memory
pressure felt by the block-device cache can transmitted to the fs, so the
fs can associate private data with the block device's cache I guess.
I don't think bd_super is sufficiently persistent reference to be usable
for sync_filesystems (it could race with unmount).
But if the block device is within a dm or md device, or is an external
journal for e.g. ext3, or is in some more complex relationship with a
filesystem, then there is no way to request that any particular cache get
flushed to the block device. And if some user-space app has cached data
for the device .... there is no way to get at that either.
If we wanted a "proper" solution for this we probably need to leverage
the 'bd_claim' concept. When a device is claimed, allow an 'operations'
structure to be provided with operations like:
flush_caches (writes out data)
purge_caches (discards clean data)
discard_caches (discards all data)
prepare_resize (is allowed to fail)
commit_resize
freeze_io
thaw_io
But flush_disk is definitely a wrong interface. It has a meaningless name
(as discussed above), and purges some caches while discarding others.
What do we *really* lose if we just revert that original patch?
Thanks,
NeilBrown
From: James Bottomley <hidden> Date: 2011-03-07 22:56:12
On Tue, 2011-03-08 at 09:44 +1100, NeilBrown wrote:
(linux-fsdevel added - seems relevant)
Heh, Christoph will be pleased.
On Mon, 07 Mar 2011 10:46:58 -0600 James Bottomley [off-list ref]
wrote:
quoted
On Sun, 2011-03-06 at 21:22 -0700, Andrew Patterson wrote:
quoted
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
quoted
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).
So it seems we have a couple of problems: the first being that
flush_disk() doesn't actually do what it says (flush the disk). If it's
just discarding all data, dirty or clean, then its use in the
grow/shrink interface is definitely wrong.
The idea is that before we complete the grow/shrink, we make sure that
the device doesn't have any errors, so we want to try to write out all
dirty buffers to make sure they still have a home. If flush_disk()
doesn't do that, then we need to use a different interface ... what's
the interface to flush a disk?
Hi James,
I *always* want to make sure that my device doesn't have any errors, not just
when it changes size... but I'm not sure that regularly flushing out data is
the right way to do it.
But maybe I still misunderstand what the real point of this is.
I actually have no idea what you're talking about now. Let me start at
the beginning:
The idea behind flushing a disk on size changes is to ensure we get
immediate detection of screw ups. (screw ups specifically means dirty
data in the lost region which we can no longer write back).
If the device is reducing in size and the FS supports this, some data
migrates from the soon to be lost blocks at the end. The point about
running a full page cache flush for the device after it has been shrunk
is supposed to be to detect a screw up (i.e. a dirty page that just lost
its backing store).
My original thought was that this only needed to be done on shrink, but
HP had some pathological case where grow was implemented as shrink
first, so it got done on all size changes.
As for the correct interface to flush a disk - there isn't one.
One doesn't flush a storage device, one flushes a cache - to a storage
device. And there is not a 1-to-1 mapping.
A block device *is* associated with one cache - one which is used for
caching accesses through /dev/XXX and also by some filesystems to cache some
metadata. You can flush this cache with sync_blockdev(). This just
flushes out dirty pages in that cache, it doesn't discard clean pages.
invalidate_bdev() can discard clean pages. Call both, and get both outcomes.
If a filesystem is mounted directly on a given block_device, then it
should have a valid s_bdev pointer and it is possible to find that filesystem
from the block_device using get_active_super(). You could then call
sync_filesystem() to flush out dirty data. There isn't really a good
interface to discard clean data. shrink_dcache_sb() does some of it, other
bits of code do other bits.
Note that a block_device also can have a pointer to a super_block (bd_super).
This does not seem to be widely used .... ext3 and ext4 use it so that memory
pressure felt by the block-device cache can transmitted to the fs, so the
fs can associate private data with the block device's cache I guess.
I don't think bd_super is sufficiently persistent reference to be usable
for sync_filesystems (it could race with unmount).
But if the block device is within a dm or md device, or is an external
journal for e.g. ext3, or is in some more complex relationship with a
filesystem, then there is no way to request that any particular cache get
flushed to the block device. And if some user-space app has cached data
for the device .... there is no way to get at that either.
If we wanted a "proper" solution for this we probably need to leverage
the 'bd_claim' concept. When a device is claimed, allow an 'operations'
structure to be provided with operations like:
flush_caches (writes out data)
purge_caches (discards clean data)
discard_caches (discards all data)
prepare_resize (is allowed to fail)
commit_resize
freeze_io
thaw_io
What makes you think we participate in the resize? Most often we just
get notified. There may or may not have been some preparation, but it's
mostly not something we participate in.
But flush_disk is definitely a wrong interface. It has a meaningless name
(as discussed above), and purges some caches while discarding others.
What do we *really* lose if we just revert that original patch?
Synchronous notification of errors. If we don't try to write everything
back immediately after the size change, we don't see dirty pages in
zapped regions until the writeout/page cache management takes it into
its head to try to clean the pages.
James
On Mon, 07 Mar 2011 16:56:12 -0600 James Bottomley [off-list ref]
wrote:
On Tue, 2011-03-08 at 09:44 +1100, NeilBrown wrote:
quoted
(linux-fsdevel added - seems relevant)
Heh, Christoph will be pleased.
quoted
On Mon, 07 Mar 2011 10:46:58 -0600 James Bottomley [off-list ref]
wrote:
quoted
On Sun, 2011-03-06 at 21:22 -0700, Andrew Patterson wrote:
quoted
On Sun, 2011-03-06 at 17:47 +1100, NeilBrown wrote:
quoted
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).
So it seems we have a couple of problems: the first being that
flush_disk() doesn't actually do what it says (flush the disk). If it's
just discarding all data, dirty or clean, then its use in the
grow/shrink interface is definitely wrong.
The idea is that before we complete the grow/shrink, we make sure that
the device doesn't have any errors, so we want to try to write out all
dirty buffers to make sure they still have a home. If flush_disk()
doesn't do that, then we need to use a different interface ... what's
the interface to flush a disk?
Hi James,
I *always* want to make sure that my device doesn't have any errors, not just
when it changes size... but I'm not sure that regularly flushing out data is
the right way to do it.
But maybe I still misunderstand what the real point of this is.
I actually have no idea what you're talking about now. Let me start at
the beginning:
Thanks!
The idea behind flushing a disk on size changes is to ensure we get
immediate detection of screw ups. (screw ups specifically means dirty
data in the lost region which we can no longer write back).
If the device is reducing in size and the FS supports this, some data
migrates from the soon to be lost blocks at the end. The point about
running a full page cache flush for the device after it has been shrunk
is supposed to be to detect a screw up (i.e. a dirty page that just lost
its backing store).
Make sense. So what you *really* want to do is actively tell the filesystem
that the device is now smaller and that is should complain loudly if that is
a problem.
You cannot directly do that, so a second best is to tell it to flush all
data in the hope that if there is a problem, then this has at least some
chance of causing it to be reported.
So you don't need to purge any caches - just flush them.
I'm having trouble seeing the relevant of the bit about "some data migrates
from the soon to be lost blocks at the end".
If you have previously requested the fs to resize downwards, then I would
expect all of this to be fully completed - maybe you are trying to detect
bugs in the filesystem's 'shrink' function? Maybe it isn't directly relevant
and you are just providing it as general context ??
My original thought was that this only needed to be done on shrink, but
HP had some pathological case where grow was implemented as shrink
first, so it got done on all size changes.
I cannot see the relevance of this to the "flush caches" approach.
If the size of the device shrinks and then grows while data is in a cache,
and then you flush, it will simply result in random data appearing
in the "new" part of the device, with no reason to expect errors.
It could be relevant if you were purging caches as well as it makes sure
you don't see stale data from before a shrink.
So you seem to be saying that "purge" is important too - correct?
quoted
As for the correct interface to flush a disk - there isn't one.
One doesn't flush a storage device, one flushes a cache - to a storage
device. And there is not a 1-to-1 mapping.
A block device *is* associated with one cache - one which is used for
caching accesses through /dev/XXX and also by some filesystems to cache some
metadata. You can flush this cache with sync_blockdev(). This just
flushes out dirty pages in that cache, it doesn't discard clean pages.
invalidate_bdev() can discard clean pages. Call both, and get both outcomes.
If a filesystem is mounted directly on a given block_device, then it
should have a valid s_bdev pointer and it is possible to find that filesystem
from the block_device using get_active_super(). You could then call
sync_filesystem() to flush out dirty data. There isn't really a good
interface to discard clean data. shrink_dcache_sb() does some of it, other
bits of code do other bits.
Note that a block_device also can have a pointer to a super_block (bd_super).
This does not seem to be widely used .... ext3 and ext4 use it so that memory
pressure felt by the block-device cache can transmitted to the fs, so the
fs can associate private data with the block device's cache I guess.
I don't think bd_super is sufficiently persistent reference to be usable
for sync_filesystems (it could race with unmount).
But if the block device is within a dm or md device, or is an external
journal for e.g. ext3, or is in some more complex relationship with a
filesystem, then there is no way to request that any particular cache get
flushed to the block device. And if some user-space app has cached data
for the device .... there is no way to get at that either.
If we wanted a "proper" solution for this we probably need to leverage
the 'bd_claim' concept. When a device is claimed, allow an 'operations'
structure to be provided with operations like:
flush_caches (writes out data)
purge_caches (discards clean data)
discard_caches (discards all data)
prepare_resize (is allowed to fail)
commit_resize
freeze_io
thaw_io
What makes you think we participate in the resize? Most often we just
get notified. There may or may not have been some preparation, but it's
mostly not something we participate in.
You may not participate in the resize, but dm and md do and the
"prepare_resize" part of the interface would be useful to them. I agree
that it might not be useful to SCSI devices.
quoted
But flush_disk is definitely a wrong interface. It has a meaningless name
(as discussed above), and purges some caches while discarding others.
What do we *really* lose if we just revert that original patch?
Synchronous notification of errors. If we don't try to write everything
back immediately after the size change, we don't see dirty pages in
zapped regions until the writeout/page cache management takes it into
its head to try to clean the pages.
So if you just want synchronous errors, I think you want:
fsync_bdev()
which calls sync_filesystem() if it can find a filesystem, else
sync_blockdev(); (sync_filesystem itself calls sync_blockdev too).
However this doesn't purge clean pages from the caches so there
might be the stale data issue.
Do you see that as a real issue?
(Just a bit of clarification: fsync_bdev flushes but does not purge.
flush_disk purges but does not flush. So with flush_disk there I don't
think you will have been getting the errors you want.
Also - fsync_bdev() takes a lot longer than flush_disk. I hope
revalidate_disk is only called from places where it is safe to block
for FS writeout ... I think it is.).
Thanks,
NeilBrown