Re: [PATCH] issue storage device flush via sync_blockdev() (was Re:Linux 2.6.29)

[Jeff Garzik]

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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/