Re: [PATCH 05/11] fuse: Trust kernel i_mtime only -v2

[Maxim Patlasov <mpatlasov@parallels.com>]

Hi Miklos,

Sorry for delay, see please inline comments below.

On 11/12/2013 08:52 PM, Miklos Szeredi wrote:
> On Thu, Oct 10, 2013 at 05:10:56PM +0400, Maxim Patlasov wrote:
>> Let the kernel maintain i_mtime locally:
>>   - clear S_NOCMTIME
>>   - implement i_op->update_time()
>>   - flush mtime on fsync and last close
>>   - update i_mtime explicitly on truncate and fallocate
>>
>> Fuse inode flag FUSE_I_MTIME_DIRTY serves as indication that local i_mtime
>> should be flushed to the server eventually.
>>
>> Changed in v2 (thanks to Brian):
>>   - renamed FUSE_I_MTIME_UPDATED to FUSE_I_MTIME_DIRTY
>>   - simplified fuse_set_mtime_local()
>>   - abandoned optimizations: clearing the flag on some operations (like
>>     direct write) is doable, but may lead to unexpected changes of
>>     user-visible mtime.
>>
>> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
>> ---
>>   fs/fuse/dir.c    |  109 ++++++++++++++++++++++++++++++++++++++++++++++++------
>>   fs/fuse/file.c   |   30 +++++++++++++--
>>   fs/fuse/fuse_i.h |    6 ++-
>>   fs/fuse/inode.c  |   13 +++++-
>>   4 files changed, 138 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index f022968..eda248b 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -857,8 +857,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
>>   	struct fuse_conn *fc = get_fuse_conn(inode);
>>   
>>   	/* see the comment in fuse_change_attributes() */
>> -	if (fc->writeback_cache && S_ISREG(inode->i_mode))
>> +	if (fc->writeback_cache && S_ISREG(inode->i_mode)) {
>>   		attr->size = i_size_read(inode);
>> +		attr->mtime = inode->i_mtime.tv_sec;
>> +		attr->mtimensec = inode->i_mtime.tv_nsec;
>> +	}
>>   
>>   	stat->dev = inode->i_sb->s_dev;
>>   	stat->ino = attr->ino;
>> @@ -1582,6 +1585,82 @@ void fuse_release_nowrite(struct inode *inode)
>>   	spin_unlock(&fc->lock);
>>   }
>>   
>> +static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>> +			      struct inode *inode,
>> +			      struct fuse_setattr_in *inarg_p,
>> +			      struct fuse_attr_out *outarg_p)
>> +{
>> +	req->in.h.opcode = FUSE_SETATTR;
>> +	req->in.h.nodeid = get_node_id(inode);
>> +	req->in.numargs = 1;
>> +	req->in.args[0].size = sizeof(*inarg_p);
>> +	req->in.args[0].value = inarg_p;
>> +	req->out.numargs = 1;
>> +	if (fc->minor < 9)
>> +		req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
>> +	else
>> +		req->out.args[0].size = sizeof(*outarg_p);
>> +	req->out.args[0].value = outarg_p;
>> +}
>> +
>> +/*
>> + * Flush inode->i_mtime to the server
>> + */
>> +int fuse_flush_mtime(struct file *file, bool nofail)
>> +{
>> +	struct inode *inode = file->f_mapping->host;
>> +	struct fuse_inode *fi = get_fuse_inode(inode);
>> +	struct fuse_conn *fc = get_fuse_conn(inode);
>> +	struct fuse_req *req = NULL;
>> +	struct fuse_setattr_in inarg;
>> +	struct fuse_attr_out outarg;
>> +	int err;
>> +
>> +	if (nofail) {
>> +		req = fuse_get_req_nofail_nopages(fc, file);
>> +	} else {
>> +		req = fuse_get_req_nopages(fc);
>> +		if (IS_ERR(req))
>> +			return PTR_ERR(req);
>> +	}
>> +
>> +	memset(&inarg, 0, sizeof(inarg));
>> +	memset(&outarg, 0, sizeof(outarg));
>> +
>> +	inarg.valid |= FATTR_MTIME;
>> +	inarg.mtime = inode->i_mtime.tv_sec;
>> +	inarg.mtimensec = inode->i_mtime.tv_nsec;
>> +
>> +	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>> +	fuse_request_send(fc, req);
>> +	err = req->out.h.error;
>> +	fuse_put_request(fc, req);
>> +
>> +	if (!err)
>> +		clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> Doing the test and the clear separately opens a huge race window when i_mtime
> modifications are bound to get lost.

No. Because the whole operation is protected by i_mutex (see 
fuse_fsync_common()).

>
>> +
>> +	return err;
>> +}
>> +
>> +/*
>> + * S_NOCMTIME is clear, so we need to update inode->i_mtime manually.
>> + */
>> +static void fuse_set_mtime_local(struct iattr *iattr, struct inode *inode)
>> +{
>> +	unsigned ivalid = iattr->ia_valid;
>> +	struct fuse_inode *fi = get_fuse_inode(inode);
>> +
>> +	if ((ivalid & ATTR_MTIME) && (ivalid & ATTR_MTIME_SET)) {
>> +		/* client fs has just set mtime to iattr->ia_mtime */
>> +		inode->i_mtime = iattr->ia_mtime;
>> +		clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> This is protected by i_mutex, so it should be safe.

Yes.

>
>> +	} else if ((ivalid & ATTR_MTIME) || (ivalid & ATTR_SIZE)) {
>> +		/* client fs doesn't know that we're updating i_mtime */
> If so, why not tell the client fs to update mtime?

Looks reasonable. I'll resend updated patch soon.

>
>> +		inode->i_mtime = current_fs_time(inode->i_sb);
>> +		set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
>> +	}
>> +}
>> +
>>   /*
>>    * Set attributes, and at the same time refresh them.
>>    *
>> @@ -1641,17 +1720,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>>   		inarg.valid |= FATTR_LOCKOWNER;
>>   		inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
>>   	}
>> -	req->in.h.opcode = FUSE_SETATTR;
>> -	req->in.h.nodeid = get_node_id(inode);
>> -	req->in.numargs = 1;
>> -	req->in.args[0].size = sizeof(inarg);
>> -	req->in.args[0].value = &inarg;
>> -	req->out.numargs = 1;
>> -	if (fc->minor < 9)
>> -		req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
>> -	else
>> -		req->out.args[0].size = sizeof(outarg);
>> -	req->out.args[0].value = &outarg;
>> +	fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>>   	fuse_request_send(fc, req);
>>   	err = req->out.h.error;
>>   	fuse_put_request(fc, req);
>> @@ -1668,6 +1737,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>>   	}
>>   
>>   	spin_lock(&fc->lock);
>> +	/* the kernel maintains i_mtime locally */
>> +	if (fc->writeback_cache && S_ISREG(inode->i_mode))
>> +		fuse_set_mtime_local(attr, inode);
>> +
>>   	fuse_change_attributes_common(inode, &outarg.attr,
>>   				      attr_timeout(&outarg));
>>   	oldsize = inode->i_size;
>> @@ -1898,6 +1971,17 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
>>   	return err;
>>   }
>>   
>> +static int fuse_update_time(struct inode *inode, struct timespec *now,
>> +			    int flags)
>> +{
>> +	if (flags & S_MTIME) {
>> +		inode->i_mtime = *now;
>> +		set_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state);
>> +		BUG_ON(!S_ISREG(inode->i_mode));
>> +	}
>> +	return 0;
>> +}
>> +
>>   static const struct inode_operations fuse_dir_inode_operations = {
>>   	.lookup		= fuse_lookup,
>>   	.mkdir		= fuse_mkdir,
>> @@ -1937,6 +2021,7 @@ static const struct inode_operations fuse_common_inode_operations = {
>>   	.getxattr	= fuse_getxattr,
>>   	.listxattr	= fuse_listxattr,
>>   	.removexattr	= fuse_removexattr,
>> +	.update_time	= fuse_update_time,
>>   };
>>   
>>   static const struct inode_operations fuse_symlink_inode_operations = {
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 333a764..eabe202 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -291,6 +291,9 @@ static int fuse_open(struct inode *inode, struct file *file)
>>   
>>   static int fuse_release(struct inode *inode, struct file *file)
>>   {
>> +	if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
> So, why not make this test_and_clear_bit()?

To be uniform with fuse_fsync_common(). See please the next comment.

>
>
>> +		fuse_flush_mtime(file, true);
>> +
>>   	fuse_release_common(file, FUSE_RELEASE);
>>   
>>   	/* return value is ignored by VFS */
>> @@ -458,6 +461,12 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>>   
>>   	fuse_sync_writes(inode);
>>   
>> +	if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state)) {
> And this too.

If mtime flush fails, the bit is kept. Another way would be to use 
test_and_clear_bit() as you suggested, but set the bit back in case of 
failure. Technically they are equivalent (due to i_mutex protection), 
but current implementation looks more natural.

Thanks,
Maxim

>
>> +		int err = fuse_flush_mtime(file, false);
>> +		if (err)
>> +			goto out;
>> +	}
>> +
>>   	req = fuse_get_req_nopages(fc);
>>   	if (IS_ERR(req)) {
>>   		err = PTR_ERR(req);
>> @@ -948,16 +957,21 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
>>   	return req->misc.write.out.size;
>>   }
>>   
>> -void fuse_write_update_size(struct inode *inode, loff_t pos)
>> +bool fuse_write_update_size(struct inode *inode, loff_t pos)
>>   {
>>   	struct fuse_conn *fc = get_fuse_conn(inode);
>>   	struct fuse_inode *fi = get_fuse_inode(inode);
>> +	bool ret = false;
>>   
>>   	spin_lock(&fc->lock);
>>   	fi->attr_version = ++fc->attr_version;
>> -	if (pos > inode->i_size)
>> +	if (pos > inode->i_size) {
>>   		i_size_write(inode, pos);
>> +		ret = true;
>> +	}
>>   	spin_unlock(&fc->lock);
>> +
>> +	return ret;
>>   }
>>   
>>   static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
>> @@ -2862,8 +2876,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
>>   		goto out;
>>   
>>   	/* we could have extended the file */
>> -	if (!(mode & FALLOC_FL_KEEP_SIZE))
>> -		fuse_write_update_size(inode, offset + length);
>> +	if (!(mode & FALLOC_FL_KEEP_SIZE)) {
>> +		bool changed = fuse_write_update_size(inode, offset + length);
>> +
>> +		if (changed && fc->writeback_cache) {
>> +			struct fuse_inode *fi = get_fuse_inode(inode);
>> +
>> +			inode->i_mtime = current_fs_time(inode->i_sb);
>> +			set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
>> +		}
>> +	}
>>   
>>   	if (mode & FALLOC_FL_PUNCH_HOLE)
>>   		truncate_pagecache_range(inode, offset, offset + length - 1);
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index a490ba3..3028b8a 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -117,6 +117,8 @@ enum {
>>   	FUSE_I_ADVISE_RDPLUS,
>>   	/** An operation changing file size is in progress  */
>>   	FUSE_I_SIZE_UNSTABLE,
>> +	/** i_mtime has been updated locally; a flush to userspace needed */
>> +	FUSE_I_MTIME_DIRTY,
>>   };
>>   
>>   struct fuse_conn;
>> @@ -870,7 +872,9 @@ long fuse_ioctl_common(struct file *file, unsigned int cmd,
>>   unsigned fuse_file_poll(struct file *file, poll_table *wait);
>>   int fuse_dev_release(struct inode *inode, struct file *file);
>>   
>> -void fuse_write_update_size(struct inode *inode, loff_t pos);
>> +bool fuse_write_update_size(struct inode *inode, loff_t pos);
>> +
>> +int fuse_flush_mtime(struct file *file, bool nofail);
>>   
>>   int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>>   		    struct file *file);
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 63818ab..253701f 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -170,8 +170,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
>>   	inode->i_blocks  = attr->blocks;
>>   	inode->i_atime.tv_sec   = attr->atime;
>>   	inode->i_atime.tv_nsec  = attr->atimensec;
>> -	inode->i_mtime.tv_sec   = attr->mtime;
>> -	inode->i_mtime.tv_nsec  = attr->mtimensec;
>> +	/* mtime from server may be stale due to local buffered write */
>> +	if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
>> +		inode->i_mtime.tv_sec   = attr->mtime;
>> +		inode->i_mtime.tv_nsec  = attr->mtimensec;
>> +	}
>>   	inode->i_ctime.tv_sec   = attr->ctime;
>>   	inode->i_ctime.tv_nsec  = attr->ctimensec;
>>   
>> @@ -250,6 +253,8 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
>>   {
>>   	inode->i_mode = attr->mode & S_IFMT;
>>   	inode->i_size = attr->size;
>> +	inode->i_mtime.tv_sec  = attr->mtime;
>> +	inode->i_mtime.tv_nsec = attr->mtimensec;
>>   	if (S_ISREG(inode->i_mode)) {
>>   		fuse_init_common(inode);
>>   		fuse_init_file_inode(inode);
>> @@ -296,7 +301,9 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>>   		return NULL;
>>   
>>   	if ((inode->i_state & I_NEW)) {
>> -		inode->i_flags |= S_NOATIME|S_NOCMTIME;
>> +		inode->i_flags |= S_NOATIME;
>> +		if (!fc->writeback_cache)
>> +			inode->i_flags |= S_NOCMTIME;
>>   		inode->i_generation = generation;
>>   		inode->i_data.backing_dev_info = &fc->bdi;
>>   		fuse_init_inode(inode, attr);
>>
>