RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Namjae Jeon]

> 
> We're already calling truncate_pagecache_range() before we attempt to
> do any actual job so there is not need to truncate pagecache once more
> using truncate_setsize() after we're finished.
> 
> Remove truncate_setsize() and replace it just with i_size_write() note
> that we're holding appropriate locks.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Hi Lukas.

I added this code by getting rewiew from Hugh.
Plz see the disscusion beween Hugh and Dave.

Hugh: But your case is different: collapse is much closer to truncation,
 and if you do not unmap the private COW'ed pages, then pages left
 behind beyond the EOF will break the spec that requires SIGBUS when
 touching there, and pages within EOF will be confusingly derived
 from file data now belonging to another offset or none (move these
 pages within the user address space? no, I don't think anon_vmas
 would allow that, and there may be no right place to move them).
 
Dave: See above - we never leave pages beyond the new EOF because setting
 the new EOF is a truncate operation that calls
 truncate_setsize(inode, newsize).
 
Hugh: Right, thanks, I now see the truncate_setsize() in the xfs case -
 though not in the ext4 case, which looks as if it's just doing an
 i_size_write() afterwards.

Dave: So that's a bug in the ext4 code ;)

truncate_setsize is not needed in case Hugh pointed out ?

Thanks!

RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2298 bytes --]

On Thu, 17 Apr 2014, Namjae Jeon wrote:

> Date: Thu, 17 Apr 2014 09:41:45 +0900
> From: Namjae Jeon <namjae.jeon@samsung.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-ext4 <linux-ext4@vger.kernel.org>
> Subject: RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse
>      range
> 
> > 
> > We're already calling truncate_pagecache_range() before we attempt to
> > do any actual job so there is not need to truncate pagecache once more
> > using truncate_setsize() after we're finished.
> > 
> > Remove truncate_setsize() and replace it just with i_size_write() note
> > that we're holding appropriate locks.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> Hi Lukas.
> 
> I added this code by getting rewiew from Hugh.
> Plz see the disscusion beween Hugh and Dave.
> 
> Hugh: But your case is different: collapse is much closer to truncation,
>  and if you do not unmap the private COW'ed pages, then pages left
>  behind beyond the EOF will break the spec that requires SIGBUS when
>  touching there, and pages within EOF will be confusingly derived
>  from file data now belonging to another offset or none (move these
>  pages within the user address space? no, I don't think anon_vmas
>  would allow that, and there may be no right place to move them).
>  
> Dave: See above - we never leave pages beyond the new EOF because setting
>  the new EOF is a truncate operation that calls
>  truncate_setsize(inode, newsize).
>  
> Hugh: Right, thanks, I now see the truncate_setsize() in the xfs case -
>  though not in the ext4 case, which looks as if it's just doing an
>  i_size_write() afterwards.
> 
> Dave: So that's a bug in the ext4 code ;)
> 
> truncate_setsize is not needed in case Hugh pointed out ?
> 
> Thanks!

That is true, we need to make sure that the page cache is coherent
with what's on disk. But we've already done that before releasing
the blocks. As I mention in the comment we're doing
truncate_pagecache_range() before removing any space. That's exactly
how it's supposed to be used. See comment in
truncate_pagecache_range().

However as I noticed we do not actually need to use
truncate_pagecache_range(), but rather truncate_pagecache() so I can
change that in my patch.

Does that make sense to everyone ?

Thanks!
-Lukas

RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Namjae Jeon]

> On Thu, 17 Apr 2014, Namjae Jeon wrote:
> 
> > Date: Thu, 17 Apr 2014 09:41:45 +0900
> > From: Namjae Jeon <namjae.jeon@samsung.com>
> > To: Lukáš Czerner <lczerner@redhat.com>
> > Cc: linux-ext4 <linux-ext4@vger.kernel.org>
> > Subject: RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse
> >      range
> >
> > >
> > > We're already calling truncate_pagecache_range() before we attempt to
> > > do any actual job so there is not need to truncate pagecache once more
> > > using truncate_setsize() after we're finished.
> > >
> > > Remove truncate_setsize() and replace it just with i_size_write() note
> > > that we're holding appropriate locks.
> > >
> > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> >
> > Hi Lukas.
> >
> > I added this code by getting rewiew from Hugh.
> > Plz see the disscusion beween Hugh and Dave.
> >
> > Hugh: But your case is different: collapse is much closer to truncation,
> >  and if you do not unmap the private COW'ed pages, then pages left
> >  behind beyond the EOF will break the spec that requires SIGBUS when
> >  touching there, and pages within EOF will be confusingly derived
> >  from file data now belonging to another offset or none (move these
> >  pages within the user address space? no, I don't think anon_vmas
> >  would allow that, and there may be no right place to move them).
> >
> > Dave: See above - we never leave pages beyond the new EOF because setting
> >  the new EOF is a truncate operation that calls
> >  truncate_setsize(inode, newsize).
> >
> > Hugh: Right, thanks, I now see the truncate_setsize() in the xfs case -
> >  though not in the ext4 case, which looks as if it's just doing an
> >  i_size_write() afterwards.
> >
> > Dave: So that's a bug in the ext4 code ;)
> >
> > truncate_setsize is not needed in case Hugh pointed out ?
> >
> > Thanks!
> 
> That is true, we need to make sure that the page cache is coherent
> with what's on disk. But we've already done that before releasing
> the blocks. As I mention in the comment we're doing
> truncate_pagecache_range() before removing any space. That's exactly
> how it's supposed to be used. See comment in
> truncate_pagecache_range().
> 
> However as I noticed we do not actually need to use
> truncate_pagecache_range(), but rather truncate_pagecache() so I can
> change that in my patch.
Hi Lukas.

Will you change that in your patch ?
Actually, I am waiting for this one..
Thanks.
> 
> Does that make sense to everyone ?
> 
> Thanks!
> -Lukas

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2999 bytes --]

On Fri, 18 Apr 2014, Namjae Jeon wrote:

> Date: Fri, 18 Apr 2014 14:31:35 +0900
> From: Namjae Jeon <namjae.jeon@samsung.com>
> To: 'Lukáš Czerner' <lczerner@redhat.com>
> Cc: 'linux-ext4' <linux-ext4@vger.kernel.org>
> Subject: RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse
>      range
> 
> > On Thu, 17 Apr 2014, Namjae Jeon wrote:
> > 
> > > Date: Thu, 17 Apr 2014 09:41:45 +0900
> > > From: Namjae Jeon <namjae.jeon@samsung.com>
> > > To: Lukáš Czerner <lczerner@redhat.com>
> > > Cc: linux-ext4 <linux-ext4@vger.kernel.org>
> > > Subject: RE: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse
> > >      range
> > >
> > > >
> > > > We're already calling truncate_pagecache_range() before we attempt to
> > > > do any actual job so there is not need to truncate pagecache once more
> > > > using truncate_setsize() after we're finished.
> > > >
> > > > Remove truncate_setsize() and replace it just with i_size_write() note
> > > > that we're holding appropriate locks.
> > > >
> > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > >
> > > Hi Lukas.
> > >
> > > I added this code by getting rewiew from Hugh.
> > > Plz see the disscusion beween Hugh and Dave.
> > >
> > > Hugh: But your case is different: collapse is much closer to truncation,
> > >  and if you do not unmap the private COW'ed pages, then pages left
> > >  behind beyond the EOF will break the spec that requires SIGBUS when
> > >  touching there, and pages within EOF will be confusingly derived
> > >  from file data now belonging to another offset or none (move these
> > >  pages within the user address space? no, I don't think anon_vmas
> > >  would allow that, and there may be no right place to move them).
> > >
> > > Dave: See above - we never leave pages beyond the new EOF because setting
> > >  the new EOF is a truncate operation that calls
> > >  truncate_setsize(inode, newsize).
> > >
> > > Hugh: Right, thanks, I now see the truncate_setsize() in the xfs case -
> > >  though not in the ext4 case, which looks as if it's just doing an
> > >  i_size_write() afterwards.
> > >
> > > Dave: So that's a bug in the ext4 code ;)
> > >
> > > truncate_setsize is not needed in case Hugh pointed out ?
> > >
> > > Thanks!
> > 
> > That is true, we need to make sure that the page cache is coherent
> > with what's on disk. But we've already done that before releasing
> > the blocks. As I mention in the comment we're doing
> > truncate_pagecache_range() before removing any space. That's exactly
> > how it's supposed to be used. See comment in
> > truncate_pagecache_range().
> > 
> > However as I noticed we do not actually need to use
> > truncate_pagecache_range(), but rather truncate_pagecache() so I can
> > change that in my patch.
> Hi Lukas.
> 
> Will you change that in your patch ?
> Actually, I am waiting for this one..
> Thanks.

Ah, sorry about that. I'll resend.

-Lukas

> > 
> > Does that make sense to everyone ?
> > 
> > Thanks!
> > -Lukas
> 
> 

[PATCH 1/5] ext4: Use filemap_write_and_wait_range() correctly in collapse range

[Lukas Czerner]

Currently we're passing -1 as lend argumnet for
filemap_write_and_wait_range() which is wrong since lend is signed type
so it would cause some confusion and we might not write_and_wait for the
entire range we're expecting to write.

Fix it by using LLONG_MAX instead.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ff823b7..821c1d4 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5378,7 +5378,7 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	punch_stop = (offset + len) >> EXT4_BLOCK_SIZE_BITS(sb);
 
 	/* Write out all dirty pages */
-	ret = filemap_write_and_wait_range(inode->i_mapping, offset, -1);
+	ret = filemap_write_and_wait_range(inode->i_mapping, offset, LLONG_MAX);
 	if (ret)
 		return ret;
 
-- 
1.8.3.1

[PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Lukas Czerner]

We're already calling truncate_pagecache_range() before we attempt to
do any actual job so there is not need to truncate pagecache once more
using truncate_setsize() after we're finished.

Remove truncate_setsize() and replace it just with i_size_write() note
that we're holding appropriate locks.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 25ed60f..9cd762c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5442,7 +5442,7 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	}
 
 	new_size = i_size_read(inode) - len;
-	truncate_setsize(inode, new_size);
+	i_size_write(inode, new_size);
 	EXT4_I(inode)->i_disksize = new_size;
 
 	ext4_discard_preallocations(inode);
-- 
1.8.3.1

Re: [PATCH 3/5] ext4: No need to truncate pagecache twice in collapse range

[Theodore Ts'o]

On Wed, Apr 16, 2014 at 08:33:00PM +0200, Lukas Czerner wrote:
> We're already calling truncate_pagecache_range() before we attempt to
> do any actual job so there is not need to truncate pagecache once more
> using truncate_setsize() after we're finished.
> 
> Remove truncate_setsize() and replace it just with i_size_write() note
> that we're holding appropriate locks.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Applied, with truncate_pagecache_range() changed to be
truncate_pagecache() in the commit description.

Thanks!!

					- Ted