Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, 21 Dec 2000, Linus Torvalds wrote: > > > On Fri, 22 Dec 2000, Jan Niehusmann wrote: > > > > This is the result - against test12-pre7, but works well with > > test13-pre3: > > This looks bogus. It is bogus. My apologies. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, Dec 21, 2000 at 05:01:00PM -0800, Linus Torvalds wrote:
>
>
> On Fri, 22 Dec 2000, Jan Niehusmann wrote:
> >
> > The test I did initially was the following:
> >
> > if(!atomic_read(>b_count) &&
> > (destroy_dirty_buffers || !buffer_dirty(bh))
> > && ! (bh->b_page && bh->b_page->mapping)
> > )
> >
> > That is, I was explicitely checking for a mapped page. It worked well, too.
> > Is this more reasonable?
>
> I'd suggest just doing this instead (warning: cut-and-paste in xterm, so
> white-space damage):
> which just ignores mapped buffers entirely (and doesn't test for
> bh->b_page being non-NULL, because that shouldn't be allowed anyway).
Yes, looks good to me, and passes some tests. Here is a patch that has not
been cut and pasted:
--- linux/fs/buffer.c.orig Thu Dec 21 20:30:03 2000
+++ linux/fs/buffer.c Fri Dec 22 02:11:29 2000
@@ -643,7 +643,12 @@
continue;
for (i = nr_buffers_type[nlist]; i > 0 ; bh = bh_next, i--) {
bh_next = bh->b_next_free;
+
+ /* Another device? */
if (bh->b_dev != dev)
+ continue;
+ /* Part of a mapping? */
+ if (bh->b_page->mapping)
continue;
if (buffer_locked(bh)) {
atomic_inc(>b_count);
I have one additional question: invalidate_buffers normally gets called if
someone wants to make sure that, after the call, read accesses to a device
really go to the device and don't get served by a cache. Is there some
mechanismn that does the same thing to mapped pages?
Jan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Fri, 22 Dec 2000, Jan Niehusmann wrote:
>
> The test I did initially was the following:
>
> if(!atomic_read(>b_count) &&
> (destroy_dirty_buffers || !buffer_dirty(bh))
> && ! (bh->b_page && bh->b_page->mapping)
> )
>
> That is, I was explicitely checking for a mapped page. It worked well, too.
> Is this more reasonable?
I'd suggest just doing this instead (warning: cut-and-paste in xterm, so
white-space damage):
--- linux/fs/buffer.c.old Wed Dec 20 17:50:56 2000
+++ linux/fs/buffer.c Thu Dec 21 16:42:11 2000
@@ -639,8 +639,13 @@
continue;
for (i = nr_buffers_type[nlist]; i > 0 ; bh = bh_next, i--) {
bh_next = bh->b_next_free;
+
+ /* Another device? */
if (bh->b_dev != dev)
continue;
+ /* Part of a mapping? */
+ if (bh->b_page->mapping)
+ continue;
if (buffer_locked(bh)) {
atomic_inc(>b_count);
spin_unlock(_list_lock);
which just ignores mapped buffers entirely (and doesn't test for
bh->b_page being non-NULL, because that shouldn't be allowed anyway).
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, Dec 21, 2000 at 04:37:30PM -0800, Linus Torvalds wrote: > This looks bogus. It may be - I just did what Al told me without really understanding it ;-) The test I did initially was the following: if(!atomic_read(>b_count) && (destroy_dirty_buffers || !buffer_dirty(bh)) && ! (bh->b_page && bh->b_page->mapping) ) That is, I was explicitely checking for a mapped page. It worked well, too. Is this more reasonable? Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Fri, 22 Dec 2000, Jan Niehusmann wrote: > > This is the result - against test12-pre7, but works well with > test13-pre3: This looks bogus. You can't test "bh->b_next!=0", because that is entirely meaningless. b_next can be NULL either because the buffer isn't hashed, or because the buffer _is_ hashed, but just happens to be last on the hash chain. So testing "bh->b_next" doesn't actually tell you anything at all. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Fri, 22 Dec 2000, Jan Niehusmann wrote: This is the result - against test12-pre7, but works well with test13-pre3: This looks bogus. You can't test "bh-b_next!=0", because that is entirely meaningless. b_next can be NULL either because the buffer isn't hashed, or because the buffer _is_ hashed, but just happens to be last on the hash chain. So testing "bh-b_next" doesn't actually tell you anything at all. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, Dec 21, 2000 at 04:37:30PM -0800, Linus Torvalds wrote: This looks bogus. It may be - I just did what Al told me without really understanding it ;-) The test I did initially was the following: if(!atomic_read(bh-b_count) (destroy_dirty_buffers || !buffer_dirty(bh)) ! (bh-b_page bh-b_page-mapping) ) That is, I was explicitely checking for a mapped page. It worked well, too. Is this more reasonable? Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Fri, 22 Dec 2000, Jan Niehusmann wrote:
The test I did initially was the following:
if(!atomic_read(bh-b_count)
(destroy_dirty_buffers || !buffer_dirty(bh))
! (bh-b_page bh-b_page-mapping)
)
That is, I was explicitely checking for a mapped page. It worked well, too.
Is this more reasonable?
I'd suggest just doing this instead (warning: cut-and-paste in xterm, so
white-space damage):
--- linux/fs/buffer.c.old Wed Dec 20 17:50:56 2000
+++ linux/fs/buffer.c Thu Dec 21 16:42:11 2000
@@ -639,8 +639,13 @@
continue;
for (i = nr_buffers_type[nlist]; i 0 ; bh = bh_next, i--) {
bh_next = bh-b_next_free;
+
+ /* Another device? */
if (bh-b_dev != dev)
continue;
+ /* Part of a mapping? */
+ if (bh-b_page-mapping)
+ continue;
if (buffer_locked(bh)) {
atomic_inc(bh-b_count);
spin_unlock(lru_list_lock);
which just ignores mapped buffers entirely (and doesn't test for
bh-b_page being non-NULL, because that shouldn't be allowed anyway).
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, 21 Dec 2000, Linus Torvalds wrote: On Fri, 22 Dec 2000, Jan Niehusmann wrote: This is the result - against test12-pre7, but works well with test13-pre3: This looks bogus. It is bogus. My apologies. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, Dec 07, 2000 at 05:26:46PM -0500, Alexander Viro wrote: > That invalidate_buffers() should leave the unhashed ones alone. If it can't > be found via getblk() - just leave it as is. > > IOW, let it skip bh if (bh->b_next == NULL && !destroy_dirty_buffers). > No warnings needed - it's a normal situation. Yes, if(bh->b_next == NULL && !destroy_dirty_buffers) seems to work, too. I wonder why, because, if I analysed the problem correctly, it was caused by the page mapping. Or is it a general rule that bh->b_next==NULL if bh->b_page->mapping != NULL, ie. a buffer is never directly hased and belongs to a mapped page? Is there a place one can look for documentation about these things? Another question is what should happen with the mapped pages? Whoever calls invalidate_buffers(), probably does it because the underlying device disappered or changed, so the page mappings should be invalidated, too. OTOH, pages are (normally) mapped through inodes, and if the inode stays valid after the invalidate_buffers() (ie. if it's called by an lvm resize event), the page mapping stays valid, too. Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, 7 Dec 2000, Udo A. Steinberg wrote: > Jan Niehusmann wrote: > > > > The following patch actually prevents the corruption I described. > > > > I'd like to hear from the people having problems with hdparm, if it helps > > them, too. > > Yes, it prevents the issue. > > > Please note that the patch circumvents the problem more than it fixes it. > > The true fix would invalidate the mappings, but I don't know how to do it. > > I don't know either. What does Alexander Viro say to all of this? That invalidate_buffers() should leave the unhashed ones alone. If it can't be found via getblk() - just leave it as is. IOW, let it skip bh if (bh->b_next == NULL && !destroy_dirty_buffers). No warnings needed - it's a normal situation. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
Jan Niehusmann wrote:
>
> The following patch actually prevents the corruption I described.
>
> I'd like to hear from the people having problems with hdparm, if it helps
> them, too.
Yes, it prevents the issue.
> Please note that the patch circumvents the problem more than it fixes it.
> The true fix would invalidate the mappings, but I don't know how to do it.
I don't know either. What does Alexander Viro say to all of this?
-Udo.
Same debug patch adapted to test12-pre7 follows:
--- linux/fs/buffer.c Thu Dec 7 22:55:54 2000
+++ /usr/src/linux/fs/buffer.c Thu Dec 7 22:49:02 2000
@@ -627,7 +627,7 @@
then an invalidate_buffers call that doesn't trash dirty buffers. */
void __invalidate_buffers(kdev_t dev, int destroy_dirty_buffers)
{
- int i, nlist, slept;
+ int i, nlist, slept, db_message = 0;
struct buffer_head * bh, * bh_next;
retry:
@@ -653,9 +653,13 @@
write_lock(_table_lock);
if (!atomic_read(>b_count) &&
(destroy_dirty_buffers || !buffer_dirty(bh))) {
- remove_inode_queue(bh);
- __remove_from_queues(bh);
- put_last_free(bh);
+ if (bh->b_page && bh->b_page->mapping)
+ db_message = 1;
+ else {
+ remove_inode_queue(bh);
+ __remove_from_queues(bh);
+ put_last_free(bh);
+ }
}
/* else complain loudly? */
@@ -668,6 +672,8 @@
spin_unlock(_list_lock);
if (slept)
goto retry;
+ if (db_message)
+ printk("invalidate_buffers with mapped page!\n");
}
void set_blocksize(kdev_t dev, int size)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
Jan Niehusmann wrote:
The following patch actually prevents the corruption I described.
I'd like to hear from the people having problems with hdparm, if it helps
them, too.
Yes, it prevents the issue.
Please note that the patch circumvents the problem more than it fixes it.
The true fix would invalidate the mappings, but I don't know how to do it.
I don't know either. What does Alexander Viro say to all of this?
-Udo.
Same debug patch adapted to test12-pre7 follows:
--- linux/fs/buffer.c Thu Dec 7 22:55:54 2000
+++ /usr/src/linux/fs/buffer.c Thu Dec 7 22:49:02 2000
@@ -627,7 +627,7 @@
then an invalidate_buffers call that doesn't trash dirty buffers. */
void __invalidate_buffers(kdev_t dev, int destroy_dirty_buffers)
{
- int i, nlist, slept;
+ int i, nlist, slept, db_message = 0;
struct buffer_head * bh, * bh_next;
retry:
@@ -653,9 +653,13 @@
write_lock(hash_table_lock);
if (!atomic_read(bh-b_count)
(destroy_dirty_buffers || !buffer_dirty(bh))) {
- remove_inode_queue(bh);
- __remove_from_queues(bh);
- put_last_free(bh);
+ if (bh-b_page bh-b_page-mapping)
+ db_message = 1;
+ else {
+ remove_inode_queue(bh);
+ __remove_from_queues(bh);
+ put_last_free(bh);
+ }
}
/* else complain loudly? */
@@ -668,6 +672,8 @@
spin_unlock(lru_list_lock);
if (slept)
goto retry;
+ if (db_message)
+ printk("invalidate_buffers with mapped page!\n");
}
void set_blocksize(kdev_t dev, int size)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, 7 Dec 2000, Udo A. Steinberg wrote: Jan Niehusmann wrote: The following patch actually prevents the corruption I described. I'd like to hear from the people having problems with hdparm, if it helps them, too. Yes, it prevents the issue. Please note that the patch circumvents the problem more than it fixes it. The true fix would invalidate the mappings, but I don't know how to do it. I don't know either. What does Alexander Viro say to all of this? That invalidate_buffers() should leave the unhashed ones alone. If it can't be found via getblk() - just leave it as is. IOW, let it skip bh if (bh-b_next == NULL !destroy_dirty_buffers). No warnings needed - it's a normal situation. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Re: fs corruption with invalidate_buffers()
On Thu, Dec 07, 2000 at 05:26:46PM -0500, Alexander Viro wrote: That invalidate_buffers() should leave the unhashed ones alone. If it can't be found via getblk() - just leave it as is. IOW, let it skip bh if (bh-b_next == NULL !destroy_dirty_buffers). No warnings needed - it's a normal situation. Yes, if(bh-b_next == NULL !destroy_dirty_buffers) seems to work, too. I wonder why, because, if I analysed the problem correctly, it was caused by the page mapping. Or is it a general rule that bh-b_next==NULL if bh-b_page-mapping != NULL, ie. a buffer is never directly hased and belongs to a mapped page? Is there a place one can look for documentation about these things? Another question is what should happen with the mapped pages? Whoever calls invalidate_buffers(), probably does it because the underlying device disappered or changed, so the page mappings should be invalidated, too. OTOH, pages are (normally) mapped through inodes, and if the inode stays valid after the invalidate_buffers() (ie. if it's called by an lvm resize event), the page mapping stays valid, too. Jan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
