On Fri, 18 Apr 2014, Namjae Jeon wrote: > Date: Fri, 18 Apr 2014 14:31:35 +0900 > From: Namjae Jeon <namjae.jeon@xxxxxxxxxxx> > To: 'Lukáš Czerner' <lczerner@xxxxxxxxxx> > Cc: 'linux-ext4' <linux-ext4@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxx> > > > To: Lukáš Czerner <lczerner@xxxxxxxxxx> > > > Cc: linux-ext4 <linux-ext4@xxxxxxxxxxxxxxx> > > > 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@xxxxxxxxxx> > > > > > > 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 > >
