[PATCH, RFC] xfs: add heuristic to flush on rename

[PATCH, RFC] xfs: add heuristic to flush on rename

[Eric Sandeen]

Add a heuristic to flush data to a file which looks like it's
going through a tmpfile/rename dance, but not fsynced.

I had a report of a system with many 0-length files after
package updates; as it turns out, the user had basically
done 'yum update' and punched the power button when it was
done.

Granted, the admin should not do this.  Granted, the package
manager should ensure persistence of files it updated.

Ext4, however, added a heuristic like this for just this case;
someone who writes file.tmp, then renames over file, but
never issues an fsync.

Now, this does smack of O_PONIES, but I would hope that it's
fairly benign.  If someone already synced the tmpfile, it's
a no-op.

And it's not THAT far off our "flush on close if the file was
truncated" heuristic.

Comments?  Flames?  Testing anyone would like to see?

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ef1ca01..5c95ef5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -371,6 +371,19 @@ xfs_vn_rename(
 	xfs_dentry_to_name(&oname, odentry, 0);
 	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
 
+	/*
+	 * If we are renaming a just-written file over an existing
+	 * file, be pedantic and flush it out if it looks like somebody
+	 * is doing a tmpfile dance, and didn't fsync.  Best effort;
+	 * ignore errors.
+	 */
+	if (new_inode) {
+		xfs_inode_t *ip = XFS_I(odentry->d_inode);
+
+		if (VN_DIRTY(VFS_I(ip)) && ip->i_delayed_blks > 0)
+			filemap_flush(new_inode->i_mapping);
+	}
+
 	return -xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
 			   XFS_I(ndir), &nname, new_inode ?
 						XFS_I(new_inode) : NULL);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Christoph Hellwig]

> +	/*
> +	 * If we are renaming a just-written file over an existing
> +	 * file, be pedantic and flush it out if it looks like somebody
> +	 * is doing a tmpfile dance, and didn't fsync.  Best effort;
> +	 * ignore errors.
> +	 */
> +	if (new_inode) {
> +		xfs_inode_t *ip = XFS_I(odentry->d_inode);
> +
> +		if (VN_DIRTY(VFS_I(ip)) && ip->i_delayed_blks > 0)
> +			filemap_flush(new_inode->i_mapping);
> +	}

IFF we want a heuristic it should be a proper filemap_write_and_wait.  a
non-blocking start of I/O without waiting for it is snake oil.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Eric Sandeen]

On 4/25/14, 2:55 PM, Christoph Hellwig wrote:
>> +	/*
>> +	 * If we are renaming a just-written file over an existing
>> +	 * file, be pedantic and flush it out if it looks like somebody
>> +	 * is doing a tmpfile dance, and didn't fsync.  Best effort;
>> +	 * ignore errors.
>> +	 */
>> +	if (new_inode) {
>> +		xfs_inode_t *ip = XFS_I(odentry->d_inode);
>> +
>> +		if (VN_DIRTY(VFS_I(ip)) && ip->i_delayed_blks > 0)
>> +			filemap_flush(new_inode->i_mapping);
>> +	}
> 
> IFF we want a heuristic it should be a proper filemap_write_and_wait.  a
> non-blocking start of I/O without waiting for it is snake oil.

Hi Christoph -

It's the same thing we do on truncated and written files, FWIW:

                truncated = xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED);
                if (truncated) {
                        xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE);
                        if (VN_DIRTY(VFS_I(ip)) && ip->i_delayed_blks > 0) {
                                error = -filemap_flush(VFS_I(ip)->i_mapping);
                                if (error)
                                        return error;
                        }
                }

and that's served us pretty well in the past years.

I don't know if we're looking for rock-solid guarantees, or just do a little
to try to save the user from themselves... blocking would make this a lot more
heavyweight I suppose.  Just depends on what we are willing / trying to do.

thanks,
-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Eric Sandeen]

On 4/25/14, 2:42 PM, Eric Sandeen wrote:
> Add a heuristic to flush data to a file which looks like it's
> going through a tmpfile/rename dance, but not fsynced.
> 
> I had a report of a system with many 0-length files after
> package updates; as it turns out, the user had basically
> done 'yum update' and punched the power button when it was
> done.
> 
> Granted, the admin should not do this.  Granted, the package
> manager should ensure persistence of files it updated.
> 
> Ext4, however, added a heuristic like this for just this case;
> someone who writes file.tmp, then renames over file, but
> never issues an fsync.
> 
> Now, this does smack of O_PONIES, but I would hope that it's
> fairly benign.  If someone already synced the tmpfile, it's
> a no-op.
> 
> And it's not THAT far off our "flush on close if the file was
> truncated" heuristic.
> 
> Comments?  Flames?  Testing anyone would like to see?
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ef1ca01..5c95ef5 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -371,6 +371,19 @@ xfs_vn_rename(
>  	xfs_dentry_to_name(&oname, odentry, 0);
>  	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
>  
> +	/*
> +	 * If we are renaming a just-written file over an existing
> +	 * file, be pedantic and flush it out if it looks like somebody
> +	 * is doing a tmpfile dance, and didn't fsync.  Best effort;
> +	 * ignore errors.
> +	 */
> +	if (new_inode) {
> +		xfs_inode_t *ip = XFS_I(odentry->d_inode);
> +
> +		if (VN_DIRTY(VFS_I(ip)) && ip->i_delayed_blks > 0)
> +			filemap_flush(new_inode->i_mapping);

Uhhh I flushed the wrong inode (thanks Brian!) but you get the idea ;)

should be:

+			filemap_flush(odentry->d_inode->i_mapping);

-Eric

> +	}
> +
>  	return -xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
>  			   XFS_I(ndir), &nname, new_inode ?
>  						XFS_I(new_inode) : NULL);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Dave Chinner]

On Fri, Apr 25, 2014 at 02:42:21PM -0500, Eric Sandeen wrote:
> Add a heuristic to flush data to a file which looks like it's
> going through a tmpfile/rename dance, but not fsynced.
> 
> I had a report of a system with many 0-length files after
> package updates; as it turns out, the user had basically
> done 'yum update' and punched the power button when it was
> done.

So yum didn't run sync() on completion of the update? That seems
rather dangerous to me - IMO system updates need to be guaranteed to
be stable by the update mechanisms, not to leave the system state to
chance if power fails or the system crashes immediately after an
update...


> Granted, the admin should not do this.  Granted, the package
> manager should ensure persistence of files it updated.

Yes, yes it should. Problem solved without needing to touch XFS.

> Ext4, however, added a heuristic like this for just this case;
> someone who writes file.tmp, then renames over file, but
> never issues an fsync.

You mean like rsync does all the time for every file it copies?

> Now, this does smack of O_PONIES, but I would hope that it's
> fairly benign.  If someone already synced the tmpfile, it's
> a no-op.

I'd suggest it will greatly impact rsync speed and have impact on
the resultant filesystem layout as it guarantees interleaving of
metadata and data on disk....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Eric Sandeen]

On 4/27/14, 4:20 PM, Dave Chinner wrote:
> On Fri, Apr 25, 2014 at 02:42:21PM -0500, Eric Sandeen wrote:
>> Add a heuristic to flush data to a file which looks like it's
>> going through a tmpfile/rename dance, but not fsynced.
>>
>> I had a report of a system with many 0-length files after
>> package updates; as it turns out, the user had basically
>> done 'yum update' and punched the power button when it was
>> done.
> 
> So yum didn't run sync() on completion of the update? That seems
> rather dangerous to me - IMO system updates need to be guaranteed to
> be stable by the update mechanisms, not to leave the system state to
> chance if power fails or the system crashes immediately after an
> update...
> 
> 
>> Granted, the admin should not do this.  Granted, the package
>> manager should ensure persistence of files it updated.
> 
> Yes, yes it should. Problem solved without needing to touch XFS.

Right, I first suggested it 5 years or so ago for RPM.  But hey, who
knows, someday maybe.

So no need to touch XFS, just every godawful userspace app out there...

Somebody should bring up the topic to wider audience, I'm sure they'll
all get fixed in short order.  Wait, or did we try that already?  :)

>> Ext4, however, added a heuristic like this for just this case;
>> someone who writes file.tmp, then renames over file, but
>> never issues an fsync.
> 
> You mean like rsync does all the time for every file it copies?

Yeah, I guess rsync doesn't fsync either.  ;)

>> Now, this does smack of O_PONIES, but I would hope that it's
>> fairly benign.  If someone already synced the tmpfile, it's
>> a no-op.
> 
> I'd suggest it will greatly impact rsync speed and have impact on
> the resultant filesystem layout as it guarantees interleaving of
> metadata and data on disk....

Ok, well, based on the responses thus far, sounds like a non-starter.

I'm not wedded to it, just thought I'd float the idea.

OTOH, it is an interesting juxtaposition to say the open O_TRUNC case
is worth catching, but the tempfile overwrite case is not.

-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Dave Chinner]

On Sun, Apr 27, 2014 at 04:56:07PM -0500, Eric Sandeen wrote:
> On 4/27/14, 4:20 PM, Dave Chinner wrote:
> > On Fri, Apr 25, 2014 at 02:42:21PM -0500, Eric Sandeen wrote:
> >> Add a heuristic to flush data to a file which looks like it's
> >> going through a tmpfile/rename dance, but not fsynced.
> >>
> >> I had a report of a system with many 0-length files after
> >> package updates; as it turns out, the user had basically
> >> done 'yum update' and punched the power button when it was
> >> done.
> > 
> > So yum didn't run sync() on completion of the update? That seems
> > rather dangerous to me - IMO system updates need to be guaranteed to
> > be stable by the update mechanisms, not to leave the system state to
> > chance if power fails or the system crashes immediately after an
> > update...
> > 
> > 
> >> Granted, the admin should not do this.  Granted, the package
> >> manager should ensure persistence of files it updated.
> > 
> > Yes, yes it should. Problem solved without needing to touch XFS.
> 
> Right, I first suggested it 5 years or so ago for RPM.  But hey, who
> knows, someday maybe.

grrrrr.

> So no need to touch XFS, just every godawful userspace app out there...
> 
> Somebody should bring up the topic to wider audience, I'm sure they'll
> all get fixed in short order.  Wait, or did we try that already?  :)

I'm not talking about any random application. Package managers are
*CRITICAL SYSTEM INFRASTRUCTURE*. They should be architectected to
handle failures gracefully; following *basic data integrity rules*
is a non-negotiable requirement for a system upgrade procedure.
Leaving the system in an indeterminate and potentially inoperable
state after a successful upgrade completion is reported is a
completely unacceptable outcome for any system management operation.

Critical infrastructure needs to Do Things Right, not require other
people to hack around it's failings and hope that they might be able
to save the system when shit goes wrong.  There is no excuse for
critical infrastructure developers failing to acknowledge and
address the data integrity requirements of their infrastructure.

> >> Ext4, however, added a heuristic like this for just this case;
> >> someone who writes file.tmp, then renames over file, but
> >> never issues an fsync.
> > 
> > You mean like rsync does all the time for every file it copies?
> 
> Yeah, I guess rsync doesn't fsync either.  ;)

That's because rsync doesn't need to sync until it completes all of
the data writes. A failed
rsync can simply be re-run after the system comes back up and
nothing is lost. That's a very different situation to a package
manager replacing binaries that the system may need to boot, yes?

> >> Now, this does smack of O_PONIES, but I would hope that it's
> >> fairly benign.  If someone already synced the tmpfile, it's
> >> a no-op.
> > 
> > I'd suggest it will greatly impact rsync speed and have impact on
> > the resultant filesystem layout as it guarantees interleaving of
> > metadata and data on disk....
> 
> Ok, well, based on the responses thus far, sounds like a non-starter.
> 
> I'm not wedded to it, just thought I'd float the idea.
> 
> OTOH, it is an interesting juxtaposition to say the open O_TRUNC case
> is worth catching, but the tempfile overwrite case is not.

We went through this years ago - the O_TRUNC case is dealing with
direct overwrite of data which we can reliably detect, usually only
occurs one file at a time, has no major performance impact and data
loss is almost entirely mitigated by the flush-on-close behaviour.
It's a pretty reliable mitigation mechanism.

Rename often involves many files (so much larger writeback delay on
async flush), it has cases we can't catch (e.g. rename of a
directory containing unsynced data files) and has much more
unpredictable behaviour (e.g. rename of files being actively written
to). There's nothing worse than having unpredictable/non-repeatable
data loss scenarios - if we can't handle all rename cases with the
same guarantees, then we shouldn't provide any data integrity
guarantees at all.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Eric Sandeen]

On 4/27/14, 6:15 PM, Dave Chinner wrote:
> On Sun, Apr 27, 2014 at 04:56:07PM -0500, Eric Sandeen wrote:
>> On 4/27/14, 4:20 PM, Dave Chinner wrote:
>>> On Fri, Apr 25, 2014 at 02:42:21PM -0500, Eric Sandeen wrote:
>>>> Add a heuristic to flush data to a file which looks like it's
>>>> going through a tmpfile/rename dance, but not fsynced.
>>>>
>>>> I had a report of a system with many 0-length files after
>>>> package updates; as it turns out, the user had basically
>>>> done 'yum update' and punched the power button when it was
>>>> done.
>>>
>>> So yum didn't run sync() on completion of the update? That seems
>>> rather dangerous to me - IMO system updates need to be guaranteed to
>>> be stable by the update mechanisms, not to leave the system state to
>>> chance if power fails or the system crashes immediately after an
>>> update...
>>>
>>>
>>>> Granted, the admin should not do this.  Granted, the package
>>>> manager should ensure persistence of files it updated.
>>>
>>> Yes, yes it should. Problem solved without needing to touch XFS.
>>
>> Right, I first suggested it 5 years or so ago for RPM.  But hey, who
>> knows, someday maybe.
> 
> grrrrr.
> 
>> So no need to touch XFS, just every godawful userspace app out there...
>>
>> Somebody should bring up the topic to wider audience, I'm sure they'll
>> all get fixed in short order.  Wait, or did we try that already?  :)
> 
> I'm not talking about any random application. Package managers are
> *CRITICAL SYSTEM INFRASTRUCTURE*. They should be architectected to
> handle failures gracefully; following *basic data integrity rules*
> is a non-negotiable requirement for a system upgrade procedure.
> Leaving the system in an indeterminate and potentially inoperable
> state after a successful upgrade completion is reported is a
> completely unacceptable outcome for any system management operation.
> 
> Critical infrastructure needs to Do Things Right, not require other
> people to hack around it's failings and hope that they might be able
> to save the system when shit goes wrong.  There is no excuse for
> critical infrastructure developers failing to acknowledge and
> address the data integrity requirements of their infrastructure.

Yeah, I know - choir, preaching, etc.

>>>> Ext4, however, added a heuristic like this for just this case;
>>>> someone who writes file.tmp, then renames over file, but
>>>> never issues an fsync.
>>>
>>> You mean like rsync does all the time for every file it copies?
>>
>> Yeah, I guess rsync doesn't fsync either.  ;)
> 
> That's because rsync doesn't need to sync until it completes all of
> the data writes. A failed
> rsync can simply be re-run after the system comes back up and
> nothing is lost. That's a very different situation to a package
> manager replacing binaries that the system may need to boot, yes?

yeah, my point is that rsync overwrites exiting files and _never_ syncs.
Not per-file, not at the end, not with any available option, AFAICT.

Different situation, yes, but arguably just as bad under the
wrong circumstances.

>>>> Now, this does smack of O_PONIES, but I would hope that it's
>>>> fairly benign.  If someone already synced the tmpfile, it's
>>>> a no-op.
>>>
>>> I'd suggest it will greatly impact rsync speed and have impact on
>>> the resultant filesystem layout as it guarantees interleaving of
>>> metadata and data on disk....
>>
>> Ok, well, based on the responses thus far, sounds like a non-starter.
>>
>> I'm not wedded to it, just thought I'd float the idea.
>>
>> OTOH, it is an interesting juxtaposition to say the open O_TRUNC case
>> is worth catching, but the tempfile overwrite case is not.
> 
> We went through this years ago - the O_TRUNC case is dealing with
> direct overwrite of data which we can reliably detect, usually only
> occurs one file at a time, has no major performance impact and data
> loss is almost entirely mitigated by the flush-on-close behaviour.
> It's a pretty reliable mitigation mechanism.

[citation needed] for a some of that, but *shrug*

> Rename often involves many files (so much larger writeback delay on
> async flush), it has cases we can't catch (e.g. rename of a
> directory containing unsynced data files) and has much more
> unpredictable behaviour (e.g. rename of files being actively written
> to). There's nothing worse than having unpredictable/non-repeatable
> data loss scenarios - if we can't handle all rename cases with the
> same guarantees, then we shouldn't provide any data integrity
> guarantees at all.

Ok, so it's a NAK.

I'm over it already,
-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs: add heuristic to flush on rename

[Dave Chinner]

On Sun, Apr 27, 2014 at 07:20:09PM -0500, Eric Sandeen wrote:
> On 4/27/14, 6:15 PM, Dave Chinner wrote:
> > On Sun, Apr 27, 2014 at 04:56:07PM -0500, Eric Sandeen wrote:
> >> On 4/27/14, 4:20 PM, Dave Chinner wrote:
> >>> On Fri, Apr 25, 2014 at 02:42:21PM -0500, Eric Sandeen wrote:
> >>>> Ext4, however, added a heuristic like this for just this case;
> >>>> someone who writes file.tmp, then renames over file, but
> >>>> never issues an fsync.
> >>>
> >>> You mean like rsync does all the time for every file it copies?
> >>
> >> Yeah, I guess rsync doesn't fsync either.  ;)
> > 
> > That's because rsync doesn't need to sync until it completes all of
> > the data writes. A failed
> > rsync can simply be re-run after the system comes back up and
> > nothing is lost. That's a very different situation to a package
> > manager replacing binaries that the system may need to boot, yes?
> 
> yeah, my point is that rsync overwrites exiting files and _never_ syncs.
> Not per-file, not at the end, not with any available option, AFAICT.

But which a user can easily add.

> Different situation, yes, but arguably just as bad under the
> wrong circumstances.

Which is why rsync provides this:

$ zcat /usr/share/doc/rsync/scripts/atomic-rsync.gz
....
This script lets you update a hierarchy of files in an atomic way by first
creating a new hierarchy (using hard-links to leverage the existing files),
and then swapping the new hierarchy into place.
....

Yes, it doesn't have a sync in it but, again, that can easily be
added. The point being is that rename safety and atomic renames are
something that can be solved at the application level....

> >>>> Now, this does smack of O_PONIES, but I would hope that it's
> >>>> fairly benign.  If someone already synced the tmpfile, it's
> >>>> a no-op.
> >>>
> >>> I'd suggest it will greatly impact rsync speed and have impact on
> >>> the resultant filesystem layout as it guarantees interleaving of
> >>> metadata and data on disk....
> >>
> >> Ok, well, based on the responses thus far, sounds like a non-starter.
> >>
> >> I'm not wedded to it, just thought I'd float the idea.
> >>
> >> OTOH, it is an interesting juxtaposition to say the open O_TRUNC case
> >> is worth catching, but the tempfile overwrite case is not.
> > 
> > We went through this years ago - the O_TRUNC case is dealing with
> > direct overwrite of data which we can reliably detect, usually only
> > occurs one file at a time, has no major performance impact and data
> > loss is almost entirely mitigated by the flush-on-close behaviour.
> > It's a pretty reliable mitigation mechanism.
> 
> [citation needed] for a some of that, but *shrug*

It was internal to SGI, mainly related to Irix, unfortunately, which
is where all this "avoid NULL files" stuff came from originally...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs