[RFC PATCH] block: all callers should check blkdev_issue_flush's return

[RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Mike Snitzer]

It is concerning that a FLUSH may fail but the blkdev_issue_flush
callers assume it will always succeed.

Each blkdev_issue_flush caller should come to terms with the reality
that a FLUSH may fail -- the file_operations' .fsync methods in
particular.  nilfs2 is the only filesystem that checks
blkdev_issue_flush's return.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 include/linux/blkdev.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ba43f40..76d6e48 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -968,7 +968,7 @@ static inline struct request *blk_map_queue_find_tag(struct blk_queue_tag *bqt,
 
 #define BLKDEV_DISCARD_SECURE  0x01    /* secure discard */
 
-extern int blkdev_issue_flush(struct block_device *, gfp_t, sector_t *);
+extern int __must_check blkdev_issue_flush(struct block_device *, gfp_t, sector_t *);
 extern int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, unsigned long flags);
 extern int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
-- 
1.7.1

Re: [RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Jeff Moyer]

Mike Snitzer <snitzer@redhat.com> writes:

> It is concerning that a FLUSH may fail but the blkdev_issue_flush
> callers assume it will always succeed.
>
> Each blkdev_issue_flush caller should come to terms with the reality
> that a FLUSH may fail -- the file_operations' .fsync methods in
> particular.  nilfs2 is the only filesystem that checks
> blkdev_issue_flush's return.

Yeah, as it stands, it looks like in many cases fsync won't return an
error if a flush fails.  That's bad.

Re: [RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Christoph Hellwig]

On Tue, Jun 26, 2012 at 11:27:25AM -0400, Mike Snitzer wrote:
> It is concerning that a FLUSH may fail but the blkdev_issue_flush
> callers assume it will always succeed.
> 
> Each blkdev_issue_flush caller should come to terms with the reality
> that a FLUSH may fail -- the file_operations' .fsync methods in
> particular.  nilfs2 is the only filesystem that checks
> blkdev_issue_flush's return.

Good spot, but it would be way better if you actually provided patches
to fix this instead of just adding more compiler warnings.

Re: [RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Mike Snitzer]

On Tue, Jun 26 2012 at 11:51am -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jun 26, 2012 at 11:27:25AM -0400, Mike Snitzer wrote:
> > It is concerning that a FLUSH may fail but the blkdev_issue_flush
> > callers assume it will always succeed.
> > 
> > Each blkdev_issue_flush caller should come to terms with the reality
> > that a FLUSH may fail -- the file_operations' .fsync methods in
> > particular.  nilfs2 is the only filesystem that checks
> > blkdev_issue_flush's return.
> 
> Good spot, but it would be way better if you actually provided patches
> to fix this instead of just adding more compiler warnings.

Alasdair pointed this issue out in response to me asserting that
blkdev_issue_flush does return non-void.  But anyway, others knowing
about this issue is half the battle. ;)

Most .fsync methods are straight-forward to convert but I'd prefer each
filesystem maintainer actively audit all blkdev_issue_flush calls.

Re: [RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Joel Becker]

On Tue, Jun 26, 2012 at 11:57:50AM -0400, Mike Snitzer wrote:
> On Tue, Jun 26 2012 at 11:51am -0400,
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Tue, Jun 26, 2012 at 11:27:25AM -0400, Mike Snitzer wrote:
> > > It is concerning that a FLUSH may fail but the blkdev_issue_flush
> > > callers assume it will always succeed.
> > > 
> > > Each blkdev_issue_flush caller should come to terms with the reality
> > > that a FLUSH may fail -- the file_operations' .fsync methods in
> > > particular.  nilfs2 is the only filesystem that checks
> > > blkdev_issue_flush's return.
> > 
> > Good spot, but it would be way better if you actually provided patches
> > to fix this instead of just adding more compiler warnings.
> 
> Alasdair pointed this issue out in response to me asserting that
> blkdev_issue_flush does return non-void.  But anyway, others knowing
> about this issue is half the battle. ;)
> 
> Most .fsync methods are straight-forward to convert but I'd prefer each
> filesystem maintainer actively audit all blkdev_issue_flush calls.

So send it out with maintainers on cc: and get Acks.  That way we have a
coherent patch series cleaning up the in-tree filesystems, rather than a
bunch of warnings for every compile until the maintainers notice.

Joel


-- 

"To announce that there must be no criticism of them president, or
 that we are to stand by the president, right or wrong, is not only
 unpatriotic and servile, but is morally treasonable to the American
 public."
	- Theodore Roosevelt

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [RFC PATCH] block: all callers should check blkdev_issue_flush's return

[Mike Snitzer]

On Sun, Jul 01 2012 at  3:28am -0400,
Joel Becker <jlbec@evilplan.org> wrote:

> On Tue, Jun 26, 2012 at 11:57:50AM -0400, Mike Snitzer wrote:
> > On Tue, Jun 26 2012 at 11:51am -0400,
> > Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > > On Tue, Jun 26, 2012 at 11:27:25AM -0400, Mike Snitzer wrote:
> > > > It is concerning that a FLUSH may fail but the blkdev_issue_flush
> > > > callers assume it will always succeed.
> > > > 
> > > > Each blkdev_issue_flush caller should come to terms with the reality
> > > > that a FLUSH may fail -- the file_operations' .fsync methods in
> > > > particular.  nilfs2 is the only filesystem that checks
> > > > blkdev_issue_flush's return.
> > > 
> > > Good spot, but it would be way better if you actually provided patches
> > > to fix this instead of just adding more compiler warnings.
> > 
> > Alasdair pointed this issue out in response to me asserting that
> > blkdev_issue_flush does return non-void.  But anyway, others knowing
> > about this issue is half the battle. ;)
> > 
> > Most .fsync methods are straight-forward to convert but I'd prefer each
> > filesystem maintainer actively audit all blkdev_issue_flush calls.
> 
> So send it out with maintainers on cc: and get Acks.  That way we have a
> coherent patch series cleaning up the in-tree filesystems, rather than a
> bunch of warnings for every compile until the maintainers notice.

Hi Joel,

I shouldn't have sent an RFC patch at all; a normal mail would've
sufficed.

My intent wasn't to have that patch go upstream.  I explained as much to
Jens when I saw him last week: I just wanted to get the issue on
filesystem developers' radar (hence the RFC).

But just because someone reports something doesn't implicitly mean they
own fixing it -- I'm unfortunately quite busy with other work.

Given you have more filesystem experience and may be more inclined to
pick over the nuance of each blkdev_issue_flush caller (and how
short-circuiting on blkdev_issue_flush failure should be handled):
please feel free to get a coherent patchset going. ;)