Re: [Review] Improve XFS error checking and propagation

[Niv Sardi <xaiki@cxhome.ath.cx>]

David Chinner <dgc@sgi.com> writes:

> On Wed, Apr 02, 2008 at 01:58:09PM +1100, Niv Sardi wrote:
>> David Chinner <dgc@sgi.com> writes:
>> > On Tue, Mar 11, 2008 at 12:04:21PM +1100, David Chinner wrote:
>> >> A recent paper at the FAST08 conference highlighted a large number
>> >> of unchecked error paths in Linux filesystems and I/O layers.  As a
>> >> subsystem, XFS had the highest aggregate numbers of bad error
>> >> propagation. A tarball which contains a quilt patch series of 32
>> >> patches aimed at improving this situation can be found here:
>> >> 
>> >> http://oss.sgi.com/~dgc/xfs/error-check/xfs-error-checking.tar.gz
>> 
>> All looks good except some minor typo-editing,
>> 
>> and
>> 
>> NOK xfs-mustcheck-quotamount.patch # need to check if can happen when forcing quotas
>> 
>> I'm not sure what happens if we really DO want quotas (specified on
>> mount line and such).
>
> The behaviour will be exactly the same as previously, because the
> error returned by xfs_qm_mount_quotas() is ignored.  i.e. if we try
> to mount with quotas and the quota mount fails, we continue (after
> issuing a warning to syslog) that quotas were not turned on.
>
> This is especially important for root filesystems with quota
> enabled....

OK, I wasn't sure.

All the rest are minor aesthetics/typos my messed up notes, and can be
ignored…

>> OK xfs-mustcheck-reset-dqcounts.patch
>> OK xfs-mustcheck-dqflushall.patch
>> OK xfs-mustcheck-acl-setmode.patch
>> OK xfs-mustcheck-search-busy.patch
>> EDITED xfs-mustcheck-compute-diff.patch # xfs_fs_cmn_err alignment
>
> That patch doesn't have any calls to xfs_fs_cmn_err() in it. Can you
> clarify, please?

Oops, the edit was for: 
-+STATIC void                           /* success (>= minlen) */
++STATIC void

as it didn't really make sense anymore.

>> OK xfs-mustcheck-bmap-adjacent.patch
>> OK xfs-mustcheck-iflush-fork.patch # less error handeling !!
>
> You can't have less error handling than intentionally ignoring
> the return from a function that can't return an error. You can
> have simpler code, though, by declaring the function void....

hum, I can't remember why I wrote that anymore, oh well… looks good now.

>> OK xfs-mustcheck-bulkstat-dinode.patch
>> OK xfs-mustcheck-quiesce-fs.patch
>> OK xfs-mustcheck-bdstrat.patch
>> OK xfs-fix-error-prototypes.patch # not error handeling related
>> OK xfs-mustcheck-acl-vremove.patch
>> OK xfs-mustcheck-icsb-disable.patch
>> OK xfs-mustcheck-ioend-unwritten.patch
>> OK xfs-mustcheck-buf-associate.patch
>> OK xfs-mustcheck-reserve-blocks.patch
>> EDITED xfs-mustcheck-bawrite.patch # xfs_fs_cmn_err alignment
>
> Which means?

That's purely aesthetic, sometimes we split the string and keep it
aligned, and sometimes we pad it left so that it fits, I prefer
splitting.

>> OK xfs-mustcheck-bdwrite.patch
>> OK xfs-mustcheck-truncate-page.patch # might be incomplete

Note to self: re-read one's notes before sending them out, I wanted to
look at why we couldn't propagate error better, but now it's all
understood =)

>> EDITED xfs-mustcheck-dqflush.patch # slight style change/typo
> Details?

-hence we nevre know if we've failed to flush a dquot to disk.
+hence we never know if we've failed to flush a dquot to disk.

and xfs_fs_cmn_err stuff.

>> OK xfs-mustcheck-reset-sbqflags.patch
>> OK xfs-mustcheck-quotaoff.patch
>> EDITED xfs-mustcheck-inactive.patch # slight style change/typo
>
> Details?

-correctly. if we fail to write the final quota off trnasaction,
+correctly. if we fail to write the final quota off transaction,

and xfs_fs_cmn_err stuff.

Cheers,
-- 
Niv Sardi