linux-kernel - Re: [PATCH] issue storage device flush via sync

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]
Message-ID: <49CAD9B3.1010208@garzik.org>
Date:	Wed, 25 Mar 2009 21:26:11 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Ric Wheeler <rwheeler@...hat.com>
CC:	Eric Sandeen <sandeen@...deen.net>,
	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>
Subject: Re: [PATCH] issue storage device flush via sync_blockdev() (was Re:
 Linux 2.6.29)

Ric Wheeler wrote:
> Eric Sandeen wrote:
>> Jeff Garzik wrote:
>>  
>>> On Wed, Mar 25, 2009 at 01:40:37PM -0700, Linus Torvalds wrote:
>>>    
>>>> On Wed, 25 Mar 2009, Jeff Garzik wrote:
>>>>      
>>>>> It is clearly possible to implement an fsync(2) that causes FLUSH 
>>>>> CACHE to be
>>>>> issued, without adding full barrier support to a filesystem.  It is 
>>>>> likely
>>>>> doable to avoid touching per-filesystem code at all, if we issue 
>>>>> the flush
>>>>> from a generic fsync(2) code path in the kernel.
>>>>>         
>>>> We could easily do that. It would even work for most cases. The 
>>>> problematic ones are where filesystems do their own disk management, 
>>>> but I guess those people can do their own fsync() management too.
>>>>
>>>> Somebody send me the patch, we can try it out.
>>>>       
>>> This is a simple step that would cover a lot of cases...  sync(2)
>>> calls sync_blockdev(), and many filesystems do as well via the generic
>>> filesystem helper file_fsync (fs/sync.c).
>>>
>>> XFS code calls sync_blockdev() a "big hammer", so I hope my patch
>>> follows with known practice.
>>>
>>> Looking over every use of sync_blockdev(), its most frequent use is
>>> through fsync(2), for the selected filesystems that use the generic
>>> file_fsync helper.
>>>
>>> Most callers of sync_blockdev() in the kernel do so infrequently,
>>> when removing and invalidating volumes (MD) or storing the superblock
>>> prior to release (put_super) in some filesystems.
>>>
>>> Compile-tested only, of course :)  But it should be work :)
>>>
>>> My main concern is some hidden area that calls sync_blockdev() with
>>> a high-enough frequency that the performance hit is bad.
>>>
>>> Signed-off-by: Jeff Garzik <jgarzik@...hat.com>
>>>
>>> diff --git a/fs/buffer.c b/fs/buffer.c
>>> index 891e1c7..7b9f74a 100644
>>> --- a/fs/buffer.c
>>> +++ b/fs/buffer.c
>>> @@ -173,9 +173,14 @@ int sync_blockdev(struct block_device *bdev)
>>>  {
>>>      int ret = 0;
>>>  
>>> -    if (bdev)
>>> -        ret = filemap_write_and_wait(bdev->bd_inode->i_mapping);
>>> -    return ret;
>>> +    if (!bdev)
>>> +        return 0;
>>> +   
>>> +    ret = filemap_write_and_wait(bdev->bd_inode->i_mapping);
>>> +    if (ret)
>>> +        return ret;
>>> +   
>>> +    return blkdev_issue_flush(bdev, NULL);
>>>  }
>>>  EXPORT_SYMBOL(sync_blockdev);
>>>     
>>
>> What about when you're running over a big raid device with
>> battery-backed cache, and you trust the cache as much as much as the
>> disks.  Wouldn't this unconditional cache flush be painful there on any
>> of the callers even if they're rare?  (fs unmounts, freezes, unmounts,
>> etc?  Or a fat filesystem on that device doing an fsync?)
>>
>> xfs, reiserfs, ext4 all avoid the blkdev flush on fsync if barriers are
>> not enabled, I think for that reason...
>>
>> (I'm assuming these raid devices still honor a cache flush request even
>> if they're battery-backed?  I dunno.)
>>
>> -Eric
>>   
> I think that Jeff's patch misses the whole need to protect transactions, 
> including meta data, in a precise way. Useful for thing like unmount, 
> not to give us strong protection for transactions or for fsync().

What do you think sync_blockdev() does?  What is its purpose?

Twofold:
(1) guarantee all user data is flushed out before a major event 
(unmount, journal close, unplug, poweroff, explosion, ...)

(2) As a sledgehammer hack for simple or legacy filesystems that do not 
wish or need the complexity of transactional protection. 
sync_blockdev() is intentionally used in lieu of complexity for the 
following filesystems: HFS, HFS+, ADFS, AFFS, FAT, bfs, UFS, NTFS, qnx4.

My patch adds needed guarantees, only for the above filesystems, where 
none were present before.


> This patch will be adding overhead here - you will still need flushing 
> at the transaction commit layer of the specific file systems to get any 
> reliable transactions.

sync_blockdev() is used as fsync(2) only in simple or legacy filesystems 
that do not want a transaction commit layer!

Read the patch :)

	Jeff


--
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/