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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 01 Apr 2009 12:52:23 +0900
From:	Fernando Luis Vázquez Cao 
	<fernando@....ntt.co.jp>
To:	Fernando Luis Vázquez Cao 
	<fernando@....ntt.co.jp>, Jeff Garzik <jeff@...zik.org>,
	Christoph Hellwig <hch@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Theodore Tso <tytso@....edu>, Ingo Molnar <mingo@...e.hu>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Arjan van de Ven <arjan@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Nick Piggin <npiggin@...e.de>, David Rees <drees76@...il.com>,
	Jesper Krogh <jesper@...gh.cc>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	chris.mason@...cle.com, tj@...nel.org, bzolnier@...il.com
Subject: Re: [PATCH 6/7] xfs: propagate issue-flush error code

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@...il.com>
>> Signed-off-by: Fernando Luis Vazquez Cao <fernando@....ntt.co.jp>
> 
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/