Re: [PATCH 6/7] xfs: propagate issue-flush error code

[Fernando Luis Vázquez Cao]

Dave Chinner wrote:
> On Mon, Mar 30, 2009 at 09:33:14PM +0900, Fernando Luis Vázquez Cao wrote:
> > blkdev_issue_flush() may fail (i.e. due to media error on FLUSH CACHE
> > command execution) so its users should check for the return value.
> >
> > (This issues was first spotted Bartlomiej Zolnierkiewicz)
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > Signed-off-by: Fernando Luis Vazquez Cao <fernando@xxxxxxxxxxxxx>
>
> I think this patch is unnecessary as well as being broken.
>
>
> > diff -urNp linux-2.6.29-orig/fs/xfs/xfs_vnodeops.c linux-2.6.29/fs/xfs/xfs_vnodeops.c
> > --- linux-2.6.29-orig/fs/xfs/xfs_vnodeops.c	2009-03-24 08:12:14.000000000 +0900
> > +++ linux-2.6.29/fs/xfs/xfs_vnodeops.c	2009-03-30 15:08:21.000000000 +0900
> > @@ -678,20 +678,20 @@ xfs_fsync(
> >  		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	}
> >
> > -	if ((ip->i_mount->m_flags & XFS_MOUNT_BARRIER) && changed) {
> > +	if (!error && (ip->i_mount->m_flags & XFS_MOUNT_BARRIER) && changed) {
>
> That is wrong. Even if there was a error, we still need to
> flush the device if it hasn't already been done.

If any of the previous writes failed there is no way to know what we are actually
flushing. When we know things went awry I do not see the point in flushing the
device since part of the data we were trying to sync might not have made it to
the device.

Anyway this is a minor nitpick/policy issue that can be easily reverted to keep
the previous behavior.

> >  		/*
> >  		 * If the log write didn't issue an ordered tag we need
> >  		 * to flush the disk cache for the data device now.
> >  		 */
> >  		if (!log_flushed)
> > -			xfs_blkdev_issue_flush(ip->i_mount->m_ddev_targp);
> > +			error = xfs_blkdev_issue_flush(ip->i_mount->m_ddev_targp);
>
> What happens if we get an EOPNOTSUPP here?
> That is a meaningless error to return to fsync()....

Please look at the code again. xfs_blkdev_issue_flush() calls blkdev_issue_flush()
which turns EOPNOTSUPP into 0 to hide that error from filesystems. It is the
non-EOPNOTSUPP errors that XFS should handle: the underlying device may support
write cache flushes and still fail to flush (due to hardware errors)!

This patch is an attempt to fix the current situation.

> >  		/*
> >  		 * If this inode is on the RT dev we need to flush that
> >  		 * cache as well.
> >  		 */
> > -		if (XFS_IS_REALTIME_INODE(ip))
> > -			xfs_blkdev_issue_flush(ip->i_mount->m_rtdev_targp);
> > +		if (!error && XFS_IS_REALTIME_INODE(ip))
> > +			error = xfs_blkdev_issue_flush(ip->i_mount->m_rtdev_targp);
>
> That is broken, too. The realtime device is a different device,
> so always should be flushed regardless of the return from the
> log device.

Does it still make sense when writes to the log have failed?

Thanks!

- Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/