[PATCH] shmem: call set_page_dirty() with locked page

[PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

Here set_page_dirty() can be moved into the page lock.

CC: Hugh Dickins <hugh@veritas.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/shmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux.orig/mm/shmem.c
+++ linux/mm/shmem.c
@@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
 	if (pos + copied > inode->i_size)
 		i_size_write(inode, pos + copied);
 
-	unlock_page(page);
 	set_page_dirty(page);
+	unlock_page(page);
 	page_cache_release(page);
 
 	return copied;

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Américo Wang]

On Tue, Jul 14, 2009 at 05:29:26PM +0800, Wu Fengguang wrote:
>Here set_page_dirty() can be moved into the page lock.
>
>CC: Hugh Dickins <hugh@veritas.com>
>Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>


>---
> mm/shmem.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>--- linux.orig/mm/shmem.c
>+++ linux/mm/shmem.c
>@@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
> 	if (pos + copied > inode->i_size)
> 		i_size_write(inode, pos + copied);
> 
>-	unlock_page(page);
> 	set_page_dirty(page);
>+	unlock_page(page);
> 	page_cache_release(page);
> 
> 	return copied;
>--
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

-- 
Live like a child, think like the god.
 

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Hugh Dickins]

On Tue, 14 Jul 2009, Wu Fengguang wrote:
> Here set_page_dirty() can be moved into the page lock.

Indeed it can, but you've forgotten to mention why you think
that would be a good thing?  All I can see is that it would
very very slightly increase the page's lock hold time, which
wouldn't be an improvement: what improvement are you making?

Hugh

> 
> CC: Hugh Dickins <hugh@veritas.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  mm/shmem.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux.orig/mm/shmem.c
> +++ linux/mm/shmem.c
> @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
>  	if (pos + copied > inode->i_size)
>  		i_size_write(inode, pos + copied);
>  
> -	unlock_page(page);
>  	set_page_dirty(page);
> +	unlock_page(page);
>  	page_cache_release(page);
>  
>  	return copied;

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

On Wed, Jul 15, 2009 at 02:24:54AM +0800, Hugh Dickins wrote:
> On Tue, 14 Jul 2009, Wu Fengguang wrote:
> > Here set_page_dirty() can be moved into the page lock.
> 
> Indeed it can, but you've forgotten to mention why you think
> that would be a good thing?  All I can see is that it would

Sorry for missing out the rational.  My problem is: the hwpoison code
must make sure whether one page can be dropped without losing data.

> very very slightly increase the page's lock hold time, which
> wouldn't be an improvement: what improvement are you making?

Yes there were nothing wrong. Just to make it align with the general
practice(not rule): pages are normally dirtied inside the page lock.

The noticeable exceptions are mapped pages and pages with buffer_heads
- they could go dirty at any time. Fortunately they will have to be
unmapped/released anyway.

shmem may not be the only remaining exception. But let's fix it first.
I'd be appreciated if someone could name some more exceptions, or some
better criterion on "the data in this page can be recovered".

Thanks,
Fengguang

> > CC: Hugh Dickins <hugh@veritas.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  mm/shmem.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux.orig/mm/shmem.c
> > +++ linux/mm/shmem.c
> > @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
> >  	if (pos + copied > inode->i_size)
> >  		i_size_write(inode, pos + copied);
> >  
> > -	unlock_page(page);
> >  	set_page_dirty(page);
> > +	unlock_page(page);
> >  	page_cache_release(page);
> >  
> >  	return copied;

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

On Wed, Jul 15, 2009 at 07:10:12PM +0800, Wu Fengguang wrote:
> On Wed, Jul 15, 2009 at 02:24:54AM +0800, Hugh Dickins wrote:
> > On Tue, 14 Jul 2009, Wu Fengguang wrote:
> > > Here set_page_dirty() can be moved into the page lock.
> > 
> > Indeed it can, but you've forgotten to mention why you think
> > that would be a good thing?  All I can see is that it would
> 
> Sorry for missing out the rational.  My problem is: the hwpoison code
> must make sure whether one page can be dropped without losing data.
> 
> > very very slightly increase the page's lock hold time, which
> > wouldn't be an improvement: what improvement are you making?
> 
> Yes there were nothing wrong. Just to make it align with the general
> practice(not rule): pages are normally dirtied inside the page lock.
> 
> The noticeable exceptions are mapped pages and pages with buffer_heads
> - they could go dirty at any time. Fortunately they will have to be
> unmapped/released anyway.
> 
> shmem may not be the only remaining exception. But let's fix it first.
> I'd be appreciated if someone could name some more exceptions, or some
> better criterion on "the data in this page can be recovered".

btrfs will also dirty its extent pages without taking the page lock.
But I'd call that metadata pages, which the hwpoison code can do
nothing helpful anyway.

Thanks,
Fengguang

> > > CC: Hugh Dickins <hugh@veritas.com>
> > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > > ---
> > >  mm/shmem.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > --- linux.orig/mm/shmem.c
> > > +++ linux/mm/shmem.c
> > > @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
> > >  	if (pos + copied > inode->i_size)
> > >  		i_size_write(inode, pos + copied);
> > >  
> > > -	unlock_page(page);
> > >  	set_page_dirty(page);
> > > +	unlock_page(page);
> > >  	page_cache_release(page);
> > >  
> > >  	return copied;

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Hugh Dickins]

On Wed, 15 Jul 2009, Wu Fengguang wrote:
> On Wed, Jul 15, 2009 at 02:24:54AM +0800, Hugh Dickins wrote:
> > On Tue, 14 Jul 2009, Wu Fengguang wrote:
> > > Here set_page_dirty() can be moved into the page lock.
> > 
> > Indeed it can, but you've forgotten to mention why you think
> > that would be a good thing?  All I can see is that it would
> 
> Sorry for missing out the rational.  My problem is: the hwpoison code
> must make sure whether one page can be dropped without losing data.

Ah, thanks: your comments here will need to go into the patch
description.  But shouldn't this patch be part of the hwpoison set?

> 
> > very very slightly increase the page's lock hold time, which
> > wouldn't be an improvement: what improvement are you making?
> 
> Yes there were nothing wrong. Just to make it align with the general
> practice(not rule): pages are normally dirtied inside the page lock.

I don't mind making shmem follow more common practice here if it makes
life easier for you; but until now there's been no reason to do so -
as you say, there's no rule to call set_page_dirty with page locked.

I wish you would distinguish between dirtying a page and marking a
page dirty: if it matters to you whether it's done inside the page
lock or not, then it matter which one you are talking about.  This
page was dirtied while the page lock was held, but it's being marked
dirty just after dropping the page lock.

What about shmem_symlink: shouldn't this patch be moving the
unlock_page down there too?

Hugh

> 
> The noticeable exceptions are mapped pages and pages with buffer_heads
> - they could go dirty at any time. Fortunately they will have to be
> unmapped/released anyway.
> 
> shmem may not be the only remaining exception. But let's fix it first.
> I'd be appreciated if someone could name some more exceptions, or some
> better criterion on "the data in this page can be recovered".
> 
> Thanks,
> Fengguang
> 
> > > CC: Hugh Dickins <hugh@veritas.com>
> > > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > > ---
> > >  mm/shmem.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > --- linux.orig/mm/shmem.c
> > > +++ linux/mm/shmem.c
> > > @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
> > >  	if (pos + copied > inode->i_size)
> > >  		i_size_write(inode, pos + copied);
> > >  
> > > -	unlock_page(page);
> > >  	set_page_dirty(page);
> > > +	unlock_page(page);
> > >  	page_cache_release(page);
> > >  
> > >  	return copied;

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

On Wed, Jul 15, 2009 at 09:18:44PM +0800, Hugh Dickins wrote:
> On Wed, 15 Jul 2009, Wu Fengguang wrote:
> > On Wed, Jul 15, 2009 at 02:24:54AM +0800, Hugh Dickins wrote:
> > > On Tue, 14 Jul 2009, Wu Fengguang wrote:
> > > > Here set_page_dirty() can be moved into the page lock.
> > > 
> > > Indeed it can, but you've forgotten to mention why you think
> > > that would be a good thing?  All I can see is that it would
> > 
> > Sorry for missing out the rational.  My problem is: the hwpoison code
> > must make sure whether one page can be dropped without losing data.
> 
> Ah, thanks: your comments here will need to go into the patch
> description.  But shouldn't this patch be part of the hwpoison set?

OK, and we'll make it into the hwpoison patchset.

> > 
> > > very very slightly increase the page's lock hold time, which
> > > wouldn't be an improvement: what improvement are you making?
> > 
> > Yes there were nothing wrong. Just to make it align with the general
> > practice(not rule): pages are normally dirtied inside the page lock.
> 
> I don't mind making shmem follow more common practice here if it makes
> life easier for you; but until now there's been no reason to do so -
> as you say, there's no rule to call set_page_dirty with page locked.

Yep, thanks.

> I wish you would distinguish between dirtying a page and marking a
> page dirty: if it matters to you whether it's done inside the page
> lock or not, then it matter which one you are talking about.  This
> page was dirtied while the page lock was held, but it's being marked
> dirty just after dropping the page lock.

Right, good concepts!

> What about shmem_symlink: shouldn't this patch be moving the
> unlock_page down there too?

Sure, thanks for the reminding!

Thanks,
Fengguang
---
shmem: call set_page_dirty() with locked page

The dirtying of page and set_page_dirty() can be moved into the page lock.

- In shmem_write_end(), the page was dirtied while the page lock was held,
  but it's being marked dirty just after dropping the page lock.
- In shmem_symlink(), both dirtying and marking can be moved into page lock.

It's valuable for the hwpoison code to know whether one bad page can be dropped
without losing data. It mainly judges by testing the PG_dirty bit after taking
the page lock. So it becomes important that the dirtying of page and the
marking of dirtiness are both done inside the page lock. Which is a common
practice, but sadly not a rule.

The noticeable exceptions are
- mapped pages
- pages with buffer_heads
The above pages could go dirty at any time. Fortunately the hwpoison will
unmap the page and release the buffer_heads beforehand anyway.

Many other types of pages (eg. metadata pages) can also be dirtied at will by
their owners, the hwpoison code cannot do meaningful things to them anyway. 
Only the dirtiness of pagecache pages owned by regular files are interested.

CC: Hugh Dickins <hugh@veritas.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/shmem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux.orig/mm/shmem.c
+++ linux/mm/shmem.c
@@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
 	if (pos + copied > inode->i_size)
 		i_size_write(inode, pos + copied);
 
-	unlock_page(page);
 	set_page_dirty(page);
+	unlock_page(page);
 	page_cache_release(page);
 
 	return copied;
@@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d
 			iput(inode);
 			return error;
 		}
-		unlock_page(page);
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_op = &shmem_symlink_inode_operations;
 		kaddr = kmap_atomic(page, KM_USER0);
 		memcpy(kaddr, symname, len);
 		kunmap_atomic(kaddr, KM_USER0);
 		set_page_dirty(page);
+		unlock_page(page);
 		page_cache_release(page);
 	}
 	if (dir->i_mode & S_ISGID)

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Hugh Dickins]

On Wed, 15 Jul 2009, Wu Fengguang wrote:
> shmem: call set_page_dirty() with locked page
> 
> The dirtying of page and set_page_dirty() can be moved into the page lock.
> 
> - In shmem_write_end(), the page was dirtied while the page lock was held,
>   but it's being marked dirty just after dropping the page lock.
> - In shmem_symlink(), both dirtying and marking can be moved into page lock.
> 
> It's valuable for the hwpoison code to know whether one bad page can be dropped
> without losing data. It mainly judges by testing the PG_dirty bit after taking
> the page lock. So it becomes important that the dirtying of page and the
> marking of dirtiness are both done inside the page lock. Which is a common
> practice, but sadly not a rule.
> 
> The noticeable exceptions are
> - mapped pages
> - pages with buffer_heads
> The above pages could go dirty at any time. Fortunately the hwpoison will
> unmap the page and release the buffer_heads beforehand anyway.
> 
> Many other types of pages (eg. metadata pages) can also be dirtied at will by
> their owners, the hwpoison code cannot do meaningful things to them anyway. 
> Only the dirtiness of pagecache pages owned by regular files are interested.
> 
> CC: Hugh Dickins <hugh@veritas.com>

Ah, those were the days... ;)

> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>

Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

> ---
>  mm/shmem.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- linux.orig/mm/shmem.c
> +++ linux/mm/shmem.c
> @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
>  	if (pos + copied > inode->i_size)
>  		i_size_write(inode, pos + copied);
>  
> -	unlock_page(page);
>  	set_page_dirty(page);
> +	unlock_page(page);
>  	page_cache_release(page);
>  
>  	return copied;
> @@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d
>  			iput(inode);
>  			return error;
>  		}
> -		unlock_page(page);
>  		inode->i_mapping->a_ops = &shmem_aops;
>  		inode->i_op = &shmem_symlink_inode_operations;
>  		kaddr = kmap_atomic(page, KM_USER0);
>  		memcpy(kaddr, symname, len);
>  		kunmap_atomic(kaddr, KM_USER0);
>  		set_page_dirty(page);
> +		unlock_page(page);
>  		page_cache_release(page);
>  	}
>  	if (dir->i_mode & S_ISGID)

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

On Wed, Jul 15, 2009 at 10:22:46PM +0800, Hugh Dickins wrote:
> On Wed, 15 Jul 2009, Wu Fengguang wrote:
> > shmem: call set_page_dirty() with locked page
> > 
> > The dirtying of page and set_page_dirty() can be moved into the page lock.
> > 
> > - In shmem_write_end(), the page was dirtied while the page lock was held,
> >   but it's being marked dirty just after dropping the page lock.
> > - In shmem_symlink(), both dirtying and marking can be moved into page lock.
> > 
> > It's valuable for the hwpoison code to know whether one bad page can be dropped
> > without losing data. It mainly judges by testing the PG_dirty bit after taking
> > the page lock. So it becomes important that the dirtying of page and the
> > marking of dirtiness are both done inside the page lock. Which is a common
> > practice, but sadly not a rule.
> > 
> > The noticeable exceptions are
> > - mapped pages
> > - pages with buffer_heads
> > The above pages could go dirty at any time. Fortunately the hwpoison will
> > unmap the page and release the buffer_heads beforehand anyway.
> > 
> > Many other types of pages (eg. metadata pages) can also be dirtied at will by
> > their owners, the hwpoison code cannot do meaningful things to them anyway. 
> > Only the dirtiness of pagecache pages owned by regular files are interested.
> > 
> > CC: Hugh Dickins <hugh@veritas.com>
> 
> Ah, those were the days... ;)

Ah yeah :)

> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> 
> Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

Thanks for you kind review!

---
shmem: call set_page_dirty() with locked page

The dirtying of page and set_page_dirty() can be moved into the page lock.

- In shmem_write_end(), the page was dirtied while the page lock was held,
  but it's being marked dirty just after dropping the page lock.
- In shmem_symlink(), both dirtying and marking can be moved into page lock.

It's valuable for the hwpoison code to know whether one bad page can be dropped
without losing data. It mainly judges by testing the PG_dirty bit after taking
the page lock. So it becomes important that the dirtying of page and the
marking of dirtiness are both done inside the page lock. Which is a common
practice, but sadly not a rule.

The noticeable exceptions are
- mapped pages
- pages with buffer_heads
The above pages could go dirty at any time. Fortunately the hwpoison will
unmap the page and release the buffer_heads beforehand anyway.

Many other types of pages (eg. metadata pages) can also be dirtied at will by
their owners, the hwpoison code cannot do meaningful things to them anyway. 
Only the dirtiness of pagecache pages owned by regular files are interested.

Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/shmem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux.orig/mm/shmem.c
+++ linux/mm/shmem.c
@@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
 	if (pos + copied > inode->i_size)
 		i_size_write(inode, pos + copied);
 
-	unlock_page(page);
 	set_page_dirty(page);
+	unlock_page(page);
 	page_cache_release(page);
 
 	return copied;
@@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d
 			iput(inode);
 			return error;
 		}
-		unlock_page(page);
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_op = &shmem_symlink_inode_operations;
 		kaddr = kmap_atomic(page, KM_USER0);
 		memcpy(kaddr, symname, len);
 		kunmap_atomic(kaddr, KM_USER0);
 		set_page_dirty(page);
+		unlock_page(page);
 		page_cache_release(page);
 	}
 	if (dir->i_mode & S_ISGID)

Re: [PATCH] shmem: call set_page_dirty() with locked page

[Wu Fengguang]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

On Tue, Jul 14, 2009 at 11:33:08PM +0800, Américo Wang wrote:
> On Tue, Jul 14, 2009 at 05:29:26PM +0800, Wu Fengguang wrote:
> >Here set_page_dirty() can be moved into the page lock.
> >
> >CC: Hugh Dickins <hugh@veritas.com>
> >Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> 
> Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

Thank you.

Andi, please take the attached patch.

Thanks,
Fengguang

[-- Attachment #2: shmem-dirty-inside-page-lock.patch --]
[-- Type: text/x-diff, Size: 2090 bytes --]

shmem: call set_page_dirty() with locked page

The dirtying of page and set_page_dirty() can be moved into the page lock.

- In shmem_write_end(), the page was dirtied while the page lock was held,
  but it's being marked dirty just after dropping the page lock.
- In shmem_symlink(), both dirtying and marking can be moved into page lock.

It's valuable for the hwpoison code to know whether one bad page can be dropped
without losing data. It mainly judges by testing the PG_dirty bit after taking
the page lock. So it becomes important that the dirtying of page and the
marking of dirtiness are both done inside the page lock. Which is a common
practice, but sadly not a rule.

The noticeable exceptions are
- mapped pages
- pages with buffer_heads
The above pages could go dirty at any time. Fortunately the hwpoison will
unmap the page and release the buffer_heads beforehand anyway.

Many other types of pages (eg. metadata pages) can also be dirtied at will by
their owners, the hwpoison code cannot do meaningful things to them anyway. 
Only the dirtiness of pagecache pages owned by regular files are interested.

Acked-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/shmem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux.orig/mm/shmem.c
+++ linux/mm/shmem.c
@@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
 	if (pos + copied > inode->i_size)
 		i_size_write(inode, pos + copied);
 
-	unlock_page(page);
 	set_page_dirty(page);
+	unlock_page(page);
 	page_cache_release(page);
 
 	return copied;
@@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d
 			iput(inode);
 			return error;
 		}
-		unlock_page(page);
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_op = &shmem_symlink_inode_operations;
 		kaddr = kmap_atomic(page, KM_USER0);
 		memcpy(kaddr, symname, len);
 		kunmap_atomic(kaddr, KM_USER0);
 		set_page_dirty(page);
+		unlock_page(page);
 		page_cache_release(page);
 	}
 	if (dir->i_mode & S_ISGID)