From: David Sterba <dsterba@suse.cz>
To: Liu Bo <bo.li.liu@oracle.com>
Cc: linux-btrfs@vger.kernel.org, fdmanana@suse.com
Subject: Re: [RFC PATCH v2 2/2] Btrfs: improve fsync for nocow file
Date: Wed, 24 Jun 2015 20:21:30 +0200 [thread overview]
Message-ID: <20150624182130.GD726@suse.cz> (raw)
In-Reply-To: <20150618032722.GB8530@localhost.localdomain>
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.
next prev parent reply other threads:[~2015-06-24 18:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 7:54 [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION Liu Bo
2015-06-17 7:54 ` [RFC PATCH v2 2/2] Btrfs: improve fsync for nocow file Liu Bo
2015-06-17 15:58 ` David Sterba
2015-06-18 3:27 ` Liu Bo
2015-06-24 18:21 ` David Sterba [this message]
2015-06-25 2:24 ` Liu Bo
2015-06-25 16:10 ` David Sterba
2015-06-17 15:33 ` [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION David Sterba
2015-06-17 15:52 ` Liu Bo
2015-06-17 17:01 ` David Sterba
2015-06-18 2:46 ` Liu Bo
2015-06-18 14:38 ` i_version vs iversion (Was: Re: [RFC PATCH v2 1/2] Btrfs: add noi_version option to disable MS_I_VERSION) David Sterba
2015-06-19 11:44 ` Karel Zak
2015-06-19 11:44 ` Karel Zak
2015-06-22 20:42 ` Dave Chinner
2015-06-22 20:42 ` Dave Chinner
2015-06-24 17:28 ` David Sterba
2015-06-23 16:32 ` Theodore Ts'o
2015-06-24 8:23 ` Liu Bo
2015-06-24 18:02 ` David Sterba
2015-06-24 23:17 ` Theodore Ts'o
2015-06-24 23:17 ` Theodore Ts'o
2015-06-24 23:59 ` Dave Chinner
2015-06-24 23:59 ` Dave Chinner
2015-06-25 18:46 ` J. Bruce Fields
2015-06-25 18:46 ` J. Bruce Fields
2015-06-25 22:12 ` Theodore Ts'o
2015-06-26 13:32 ` J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150624182130.GD726@suse.cz \
--to=dsterba@suse.cz \
--cc=bo.li.liu@oracle.com \
--cc=fdmanana@suse.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.