All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: jdike@addtoit.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, hch@infradead.org,
	akpm@linux-foundation.org, viro@zeniv.linux.org.uk
Subject: Re: per-bdi-throttling: synchronous writepage doesn't work correctly
Date: Thu, 01 Nov 2007 20:55:37 +0100	[thread overview]
Message-ID: <1193946937.5911.10.camel@lappy> (raw)
In-Reply-To: <E1InfZx-0004Eu-00@dorka.pomaz.szeredi.hu>


On Thu, 2007-11-01 at 20:19 +0100, Miklos Szeredi wrote:
> > > 
> > >       See the file "Locking" for more details.
> > > 
> > > 
> > > The "should set PG_Writeback" bit threw me off I guess.
> > 
> > Hmm, set_page_writeback() is also the one clearing the radix tree dirty
> > tag. So if that is not called, we get in a bit of a mess, no?
> > 
> > Which makes me think hostfs is buggy.
> 
> Yes, looks like that sort of usage is not valid.  But not clearing the
> dirty tag won't cause any malfunction, it'll just waste some CPU when
> looking for dirty pages to write back.  This is probably why this
> wasn't noticed earlier.

Documentation/filesystems/Locking is also quite clear on the need to
call set_page_writeback() and end_page_writeback().

minimal fix for hostfs

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 8966b05..b6c1e12 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -415,6 +415,7 @@ int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 	int end_index = inode->i_size >> PAGE_CACHE_SHIFT;
 	int err;
 
+	set_page_writeback(page);
 	if (page->index >= end_index)
 		count = inode->i_size & (PAGE_CACHE_SIZE-1);
 
@@ -438,6 +439,7 @@ int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 	kunmap(page);
 
 	unlock_page(page);
+	end_page_writeback(page);
 	return err;
 }
 



WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: jdike@addtoit.com, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, hch@infradead.org,
	akpm@linux-foundation.org, viro@zeniv.linux.org.uk
Subject: Re: per-bdi-throttling: synchronous writepage doesn't work correctly
Date: Thu, 01 Nov 2007 20:55:37 +0100	[thread overview]
Message-ID: <1193946937.5911.10.camel@lappy> (raw)
In-Reply-To: <E1InfZx-0004Eu-00@dorka.pomaz.szeredi.hu>

On Thu, 2007-11-01 at 20:19 +0100, Miklos Szeredi wrote:
> > > 
> > >       See the file "Locking" for more details.
> > > 
> > > 
> > > The "should set PG_Writeback" bit threw me off I guess.
> > 
> > Hmm, set_page_writeback() is also the one clearing the radix tree dirty
> > tag. So if that is not called, we get in a bit of a mess, no?
> > 
> > Which makes me think hostfs is buggy.
> 
> Yes, looks like that sort of usage is not valid.  But not clearing the
> dirty tag won't cause any malfunction, it'll just waste some CPU when
> looking for dirty pages to write back.  This is probably why this
> wasn't noticed earlier.

Documentation/filesystems/Locking is also quite clear on the need to
call set_page_writeback() and end_page_writeback().

minimal fix for hostfs

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 8966b05..b6c1e12 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -415,6 +415,7 @@ int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 	int end_index = inode->i_size >> PAGE_CACHE_SHIFT;
 	int err;
 
+	set_page_writeback(page);
 	if (page->index >= end_index)
 		count = inode->i_size & (PAGE_CACHE_SIZE-1);
 
@@ -438,6 +439,7 @@ int hostfs_writepage(struct page *page, struct writeback_control *wbc)
 	kunmap(page);
 
 	unlock_page(page);
+	end_page_writeback(page);
 	return err;
 }
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2007-11-01 19:56 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-11-01 16:49 per-bdi-throttling: synchronous writepage doesn't work correctly Miklos Szeredi
2007-11-01 16:49 ` Miklos Szeredi
2007-11-01 16:51 ` Peter Zijlstra
2007-11-01 17:00   ` Miklos Szeredi
2007-11-01 17:00     ` Miklos Szeredi
2007-11-01 17:09     ` Peter Zijlstra
2007-11-01 17:16       ` Peter Zijlstra
2007-11-01 17:16         ` Peter Zijlstra
2007-11-01 18:35         ` Peter Zijlstra
2007-11-01 18:35           ` Peter Zijlstra
2007-11-01 19:19           ` Miklos Szeredi
2007-11-01 19:19             ` Miklos Szeredi
2007-11-01 19:55             ` Peter Zijlstra [this message]
2007-11-01 19:55               ` Peter Zijlstra
2007-11-01 17:28       ` Miklos Szeredi
2007-11-01 17:28         ` Miklos Szeredi
2007-11-01 17:39         ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1193946937.5911.10.camel@lappy \
    --to=peterz@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=hch@infradead.org \
    --cc=jdike@addtoit.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=miklos@szeredi.hu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.