On Thu, Apr 02, 2020 at 07:56:48AM -0700, Christoph Hellwig wrote: > On Thu, Apr 02, 2020 at 07:53:44AM -0700, Darrick J. Wong wrote: > > > > Looks reasonable. That being said I really hate the way we handle > > > > this - I've been wanting to rework the wsync/dirsync code to just mark > > > > as transaction as dirsync or wsync and then let xfs_trans_commit handle > > > > checking if the file system is mounted with the option to clean this > > > > mess up. Let me see if I could resurrect that quickly. > > > > > > Resurrected and under testing now. While forward porting your patch > > > I noticed it could be much simpler even without the refactor by just > > > using xfs_trans_set_sync. The downside of that is that the log force > > > is under the inode locks, but so are the log forces for all other wysnc > > > induced log forces. So I think you should just submit this in the > > > simplified version matching the rest of the wsync users as a fix. If > > > we want to optimize it later on that should be done as a separate patch > > > and for all wsync/dirsync users. > > > > Can you please send in a Reviewed-by so I can get this moving? :) > > In case the above wasn't clear: I don't think this is the right way > to go. Just to fix the reflink vs wsync bug I think we want a > one-liner like this: Sorry, I thought "you should just submit this in the simplified version" was referring to the first patch I sent, as opposed to the cleanups you're testing. > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > index b0ce04ffd3cd..e2cc7b84ca6c 100644 > --- a/fs/xfs/xfs_reflink.c > +++ b/fs/xfs/xfs_reflink.c > @@ -948,6 +948,7 @@ xfs_reflink_update_dest( > > xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE); > > + xfs_trans_set_sync(tp); This isn't enough because this is only the last transaction in the reflink sequence if we have to set the destination inode's size. If (say) we're reflinking a range inside EOF of two files that were already sharing blocks, we still won't force the log out. The other thing I thought of was simply invoking fsync after dropping the iolock, but that seemed like more work than was strictly necessary to land the reflink transactions on disk. --D > error = xfs_trans_commit(tp); > if (error) > goto out_error;
