Re: [PATCH] nilfs2: add logic for the case of file growing (old size <= new size) in nilfs_truncate()

[Vyacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2013-04-25 at 00:48 +0900, Ryusuke Konishi wrote:
> Hi Vyacheslav,
> On Wed, 24 Apr 2013 15:44:26 +0400, Vyacheslav Dubeyko wrote:
> > From: Vyacheslav Dubeyko <slava@dubeyko.com>
> > Subject: [PATCH] nilfs2: add logic for the case of file growing (old size <= new size) in nilfs_truncate()
> > 
> > There are situations when nilfs_truncate() is called with new value of i_size that is greater than old one. This patch adds logic for such case.
> > 
> > Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
> > CC: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
> 
> Did you confirm that nilfs_truncate has real problem in such situtation?
> 

I haven't any reproducing path that can reveal real problem for such
situation. But I think that, as minimum, it makes sense to return from
nilfs_truncate() without any activity for the case of blkoff ==
inode->i_blocks.

> I think hole blocks should be appended in that case.
> 
> Doesn't the current implementation work so?
> 

As I understand, in current NILFS2 implementation nilfs_truncate()
results in calling nilfs_get_block() with create flag equals by zero.
The nilfs_get_block() simply returns without any activity for the case
of file growing:


135         } else if (ret == -ENOENT) {
136                 /* not found is not error (e.g. hole); must return without
137                    the mapped state flag. */
138                 ;
139         }

Suggested implementation calls nilfs_get_block() with create flag equals
by 1, in result. So, it will allocate the block really. If the real
allocation is not desirable then I am wrong with suggested
implementation for the case of blkoff > inode->i_blocks.

Could you share your vision about it?

With the best regards,
Vyacheslav Dubeyko.

> 
> With regards,
> Ryusuke Konishi
> 
> > ---
> >  fs/nilfs2/inode.c |   30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> > index 6b49f14..5ccaace 100644
> > --- a/fs/nilfs2/inode.c
> > +++ b/fs/nilfs2/inode.c
> > @@ -698,6 +698,36 @@ void nilfs_truncate(struct inode *inode)
> >  
> >  	blocksize = sb->s_blocksize;
> >  	blkoff = (inode->i_size + blocksize - 1) >> sb->s_blocksize_bits;
> > +
> > +	if (blkoff > inode->i_blocks) {
> > +		int err;
> > +		struct address_space *mapping = inode->i_mapping;
> > +		struct page *page;
> > +		void *fsdata;
> > +		loff_t size = inode->i_size;
> > +
> > +		err = pagecache_write_begin(NULL, mapping, size, 0,
> > +						AOP_FLAG_UNINTERRUPTIBLE,
> > +						&page, &fsdata);
> > +		if (err) {
> > +			printk(KERN_ERR
> > +			    "NILFS: pagecache_write_begin() failed: err %d",
> > +			    err);
> > +			return;
> > +		}
> > +		err = pagecache_write_end(NULL, mapping, size,
> > +						0, 0, page, fsdata);
> > +		if (err < 0) {
> > +			printk(KERN_ERR
> > +			    "NILFS: pagecache_write_end() failed: err %d",
> > +			    err);
> > +			return;
> > +		}
> > +		mark_inode_dirty(inode);
> > +		return;
> > +	} else if (blkoff == inode->i_blocks)
> > +		return;
> > +
> >  	nilfs_transaction_begin(sb, &ti, 0); /* never fails */
> >  
> >  	block_truncate_page(inode->i_mapping, inode->i_size, nilfs_get_block);
> > -- 
> > 1.7.9.5
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html