| Message ID | 20150915095412.GD13399@xzibit.linux.bs1.fc.nec.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> This patch adds filemap_fdatawait_keep_errors() for call sites where > writeback error is not handled so that they don't clear error status. Patch looks good to me. Acked-by: Andi Kleen <ak@linux.intel.com> -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Junichi. On Tue, Sep 15, 2015 at 09:54:13AM +0000, Junichi Nomura wrote: > filemap_fdatawait() is a function to wait for on-going writeback > to complete but also consume and clear error status of the mapping > set during writeback. > The latter functionality is critical for applications to detect > writeback error with system calls like fsync(2)/fdatasync(2). > > However filemap_fdatawait() is also used by sync(2) or FIFREEZE > ioctl, which don't check error status of individual mappings. > > As a result, fsync() may not be able to detect writeback error > if events happen in the following order: > > Application System admin > ---------------------------------------------------------- > write data on page cache > Run sync command > writeback completes with error > filemap_fdatawait() clears error > fsync returns success > (but the data is not on disk) > > This patch adds filemap_fdatawait_keep_errors() for call sites where > writeback error is not handled so that they don't clear error status. Is this an actual problem? Write errors usually indicate that the underlying device is completely hosed and the kernel tends to make a lot of noise throughout the different layers and it often pretty quickly leads to failures of metadata IOs which which results in damage-control actions like RO remounts, so in most cases the specifics of failure handling don't end up mattering all that much. That said, no reason to not improve upon it. > @@ -2121,7 +2121,13 @@ static void wait_sb_inodes(struct super_block *sb) > iput(old_inode); > old_inode = inode; > > - filemap_fdatawait(mapping); > + /* > + * Wait for on-going writeback to complete > + * but not consume error status on this mapping. ^don't > + * Otherwise application may fail to catch writeback error mapping; otherwise, > + * using fsync(2). > + */ Can you please re-flow the comment so that it's filling up to, say, 72 or 76 or whatever column? > + filemap_fdatawait_keep_errors(mapping); > > cond_resched(); > > diff --git a/fs/sync.c b/fs/sync.c > index fbc98ee..e2b7a77 100644 > --- a/fs/sync.c > +++ b/fs/sync.c > @@ -86,7 +86,7 @@ static void fdatawrite_one_bdev(struct block_device *bdev, void *arg) > > static void fdatawait_one_bdev(struct block_device *bdev, void *arg) > { > - filemap_fdatawait(bdev->bd_inode->i_mapping); > + filemap_fdatawait_keep_errors(bdev->bd_inode->i_mapping); Maybe it'd be better to describe what's going on in detail in the function comment of filemat_fdatawait_keep_errors() and refer to that from its callers? > @@ -382,6 +391,26 @@ out: > } > EXPORT_SYMBOL(filemap_fdatawait_range); > > +/* > + * As filemap_check_errors() consumes and clears error status of mapping, > + * filemap_fdatawait() should be used only when the caller is responsible > + * for handling the error. Please make this a proper function comment. > + * > + * Use filemap_fdatawait_keep_errors() if callers just want to wait for > + * witeback and don't handle errors themselves. writeback > + * Expected call sites are system-wide / filesystem-wide data flushers: > + * e.g. sync(2), fsfreeze(8) Ditto, please either break up paragraphs or reflow to column consistently. > + */ > +void filemap_fdatawait_keep_errors(struct address_space *mapping) > +{ > + loff_t i_size = i_size_read(mapping->host); > + > + if (i_size == 0) > + return; > + > + __filemap_fdatawait_range(mapping, 0, i_size - 1); > +} Generally looks good to me. We may end up deferring writeback error from way earlier to later fsync callers but given that sync(2) doesn't even have a return value, it looks like that's the best we can do. Thanks.
> Is this an actual problem? Write errors usually indicate that the
There are transaction systems that need to track errors at the level
of the individual IO. So yes we should guarantee that if a particular
sync failed an error is always returned.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 15 Sep 2015 16:37:24 +0200 Andi Kleen <andi@firstfloor.org> wrote: > > This patch adds filemap_fdatawait_keep_errors() for call sites where > > writeback error is not handled so that they don't clear error status. > > Patch looks good to me. > Me too. It would be nice to capture the test case(s) somewhere permanent. Possibly in tools/testing/selftests, but selftests is more for peculiar linux-specific things. LTP or xfstests would be a better place. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/16/15 07:02, Andrew Morton wrote: > It would be nice to capture the test case(s) somewhere permanent. > Possibly in tools/testing/selftests, but selftests is more for peculiar > linux-specific things. LTP or xfstests would be a better place. I'll check xfstests if I can adapt the test case for its framework.
On 09/16/15 00:20, Tejun Heo wrote: >> @@ -2121,7 +2121,13 @@ static void wait_sb_inodes(struct super_block *sb) >> iput(old_inode); >> old_inode = inode; >> >> - filemap_fdatawait(mapping); >> + /* >> + * Wait for on-going writeback to complete >> + * but not consume error status on this mapping. > ^don't > >> + * Otherwise application may fail to catch writeback error > > mapping; otherwise, > >> + * using fsync(2). >> + */ > > Can you please re-flow the comment so that it's filling up to, say, 72 > or 76 or whatever column? I'll fix them. >> - filemap_fdatawait(bdev->bd_inode->i_mapping); >> + filemap_fdatawait_keep_errors(bdev->bd_inode->i_mapping); > > Maybe it'd be better to describe what's going on in detail in the > function comment of filemat_fdatawait_keep_errors() and refer to that > from its callers? Thanks, that seems better. I'll also extend function comments of filemap_fdatawait so that the difference becomes clear.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 587ac08..df52aad 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -2121,7 +2121,13 @@ static void wait_sb_inodes(struct super_block *sb) iput(old_inode); old_inode = inode; - filemap_fdatawait(mapping); + /* + * Wait for on-going writeback to complete + * but not consume error status on this mapping. + * Otherwise application may fail to catch writeback error + * using fsync(2). + */ + filemap_fdatawait_keep_errors(mapping); cond_resched(); diff --git a/fs/sync.c b/fs/sync.c index fbc98ee..e2b7a77 100644 --- a/fs/sync.c +++ b/fs/sync.c @@ -86,7 +86,7 @@ static void fdatawrite_one_bdev(struct block_device *bdev, void *arg) static void fdatawait_one_bdev(struct block_device *bdev, void *arg) { - filemap_fdatawait(bdev->bd_inode->i_mapping); + filemap_fdatawait_keep_errors(bdev->bd_inode->i_mapping); } /* diff --git a/include/linux/fs.h b/include/linux/fs.h index 72d8a84..9355f37 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2422,6 +2422,7 @@ extern int write_inode_now(struct inode *, int); extern int filemap_fdatawrite(struct address_space *); extern int filemap_flush(struct address_space *); extern int filemap_fdatawait(struct address_space *); +extern void filemap_fdatawait_keep_errors(struct address_space *); extern int filemap_fdatawait_range(struct address_space *, loff_t lstart, loff_t lend); extern int filemap_write_and_wait(struct address_space *mapping); diff --git a/mm/filemap.c b/mm/filemap.c index 72940fb..059050a 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -340,14 +340,14 @@ EXPORT_SYMBOL(filemap_flush); * Walk the list of under-writeback pages of the given address space * in the given range and wait for all of them. */ -int filemap_fdatawait_range(struct address_space *mapping, loff_t start_byte, - loff_t end_byte) +static int __filemap_fdatawait_range(struct address_space *mapping, + loff_t start_byte, loff_t end_byte) { pgoff_t index = start_byte >> PAGE_CACHE_SHIFT; pgoff_t end = end_byte >> PAGE_CACHE_SHIFT; struct pagevec pvec; int nr_pages; - int ret2, ret = 0; + int ret = 0; if (end_byte < start_byte) goto out; @@ -374,6 +374,15 @@ int filemap_fdatawait_range(struct address_space *mapping, loff_t start_byte, cond_resched(); } out: + return ret; +} + +int filemap_fdatawait_range(struct address_space *mapping, loff_t start_byte, + loff_t end_byte) +{ + int ret, ret2; + + ret = __filemap_fdatawait_range(mapping, start_byte, end_byte); ret2 = filemap_check_errors(mapping); if (!ret) ret = ret2; @@ -382,6 +391,26 @@ out: } EXPORT_SYMBOL(filemap_fdatawait_range); +/* + * As filemap_check_errors() consumes and clears error status of mapping, + * filemap_fdatawait() should be used only when the caller is responsible + * for handling the error. + * + * Use filemap_fdatawait_keep_errors() if callers just want to wait for + * witeback and don't handle errors themselves. + * Expected call sites are system-wide / filesystem-wide data flushers: + * e.g. sync(2), fsfreeze(8) + */ +void filemap_fdatawait_keep_errors(struct address_space *mapping) +{ + loff_t i_size = i_size_read(mapping->host); + + if (i_size == 0) + return; + + __filemap_fdatawait_range(mapping, 0, i_size - 1); +} + /** * filemap_fdatawait - wait for all under-writeback pages to complete * @mapping: address space structure to wait for
filemap_fdatawait() is a function to wait for on-going writeback to complete but also consume and clear error status of the mapping set during writeback. The latter functionality is critical for applications to detect writeback error with system calls like fsync(2)/fdatasync(2). However filemap_fdatawait() is also used by sync(2) or FIFREEZE ioctl, which don't check error status of individual mappings. As a result, fsync() may not be able to detect writeback error if events happen in the following order: Application System admin ---------------------------------------------------------- write data on page cache Run sync command writeback completes with error filemap_fdatawait() clears error fsync returns success (but the data is not on disk) This patch adds filemap_fdatawait_keep_errors() for call sites where writeback error is not handled so that they don't clear error status. Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com> --- fs/fs-writeback.c | 8 +++++++- fs/sync.c | 2 +- include/linux/fs.h | 1 + mm/filemap.c | 35 ++++++++++++++++++++++++++++++++--- 4 files changed, 41 insertions(+), 5 deletions(-)