On Thu, Jun 18, 2015 at 11:27:24AM +0800, Liu Bo wrote:
> > > #define BTRFS_INODE_IN_DELALLOC_LIST 9
> > > #define BTRFS_INODE_READDIO_NEED_LOCK 10
> > > #define BTRFS_INODE_HAS_PROPS 11
> > > +#define BTRFS_INODE_NOTIMESTAMP 12
> > > +#define BTRFS_INODE_NOISIZE 13
> >
> > It's not clear what the flags mean and that they're related to syncing
> > under some conditions.
>
> What do you think about BTRFS_ILOG_NOTIMESTAMP and BTRFS_ILOG_NOISIZE?
I'd say BTRFS_INODE_FSYNC_NOTIMESTAMP and BTRFS_INODE_FSYNC_NOSIZE
> > > @@ -1983,6 +2001,32 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > > goto out;
> > > }
> > >
> > > + if (BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) {
> > > + if (test_and_clear_bit(BTRFS_INODE_NOTIMESTAMP,
> > > + &BTRFS_I(inode)->runtime_flags) &&
> > > + test_and_clear_bit(BTRFS_INODE_NOISIZE,
> > > + &BTRFS_I(inode)->runtime_flags)) {
> > > +
> > > + /* make sure data is on disk and catch error */
> > > + if (!full_sync)
> > > + ret = filemap_fdatawait_range(inode->i_mapping,
> > > + start, end);
> > > +
> > > + if (!ret && !btrfs_test_opt(root, NOBARRIER)) {
> > > + mutex_lock(&root->fs_info->
> > > + fs_devices->device_list_mutex);
> > > + ret = barrier_all_devices(root->fs_info);
> >
> > Calling barrier devices at this point looks very fishy, taking global
> > device locks to sync one file as well. All files in the filesystem will
> > pay the penalty for just one nodatacow file that's being synced.
>
> Well, I'm afraid this is necessary as this is a fsync, an expensive operation,
> in the normal case, each fsync issues a superblock flush which calls barrier devices.
>
> I was expecting to not take the global device lock but btrfs is able to
> manage multiple devices which requires us to do so.
I've read the code again and came to the same conclusion, objections
withdrawn.
> > I'm not sure that handling the NOISIZE bit is safe regarding size
> > extending and sync, ie. if it's properly synchronized with i_mutex from
> > all contexts.
>
> That's also my concern, but the worst case is that someone clears
> NOISIZE bit and we continue on the normal fsync path.
Sounds safe.
> And this NOISIZE bit not only stands for i_size change, but also will be
> cleared when we do COW, I'm not sure if we need to use another bit for
> the COW change or not.
I'm not sure I understand, you mean split the NOISIZE into two bits and
use NOISIZE just for inode size change and the other one for the
cow_file_range case?
Btw, shouln't the NOISIZE bit get cleared inside cow_file_range? Both
calls are in run_delalloc_nocow, this makes sense, but I'm a bit worried
that we could forget to add it somewhere else. I don't think this would
hurt performance, cow_file_range is pretty big.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
