Re: [PATCH] bcache: only recovery I/O error for writethrough mode

Eric Wheeler
2017-07-13T00:53:55Z

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3765 bytes --]

On Wed, 12 Jul 2017, Coly Li wrote:

> On 2017/7/12 上午10:01, tang.junhui@zte.com.cn wrote:
> >>I meant "it is very necessary for data base applications which always
> >>use *writeback* mode and not switch to other mode during all their
> >>online time." ^_^
> >
> > I know, it is necessary, but not enough. who can promise they will not
> > switch during online time? This patch is logical imperfectly.
>
> Yes, I agree with you. Since Eric mentions dirty data map, an improved
> fix shows up in my head,
>
> When cache device disconnected from system,
> 0) If in non mode, do nothing.

Does non mode guarantee that nothing is dirty? I'm not convinced of that.
I think you can set non mode with dirty blocks. (Correct me if I'm wrong
here.)

> 1) If in writeback/writethough/writearound mode, and dirty map is clean,
> - switch to non mode
> - continue to handle I/O without cache device

Sure, that makes sense.

> 2) If in writeback mode, and dirty map is not clean,

You would want to do a dirty map lookup for each IO. How about this:

2) If in _any_ mode, and dirty map is dirty for *that specific block*:

If WRITE request completely overlaps the dirty segment then clear
the dirty flag and pass through to the backing dev.
otherwise:
- return -EIO immediately for WRITE request
- return -EIO immediately for READ request (*)

If the WRITE request completely overlaps the dirty segment as indicated
from the in-memory metadata, then clear its dirty flag and write to the
backing device. Whatever was dirty isn't important anymore as it was
overwritten.

Unless there is a good reason to diverge, we would want this recovery
logic would be the same for failed IOs from an existing cachedev (eg, with
badblocks), and for cachedevs that are altogether missing.

> 3) If not in writeback mode, and dirty map is not clean. It means the
> cache mode is switched from writeback mode with dirty data lost, then
> - returns -EIO immediately for WRITE request
> - returns -EIO immediately for READ request (*)

For #2,3, do a dirty map lookup for every IO: if the block is clean, pass
it to the backing device. Only -EIO if the request cannot be recovered
(block is dirty) and invoke pr_crit_once() to notify the user. We want
all IO requests to succeed to the extent possible.

I think #3 is the same case as #2. The logic is the same whether its is
now or ever was in writeback mode, regardless of the current mode.

> (*) NOTE:
> A sysfs entry "recovery_io_error" can be add here, which is disabled as
> default. If it is enabled, if a READ request does not hit dirty map,
> bcache will provide it from backing device.

Resilience first! This should default on.

Sometimes systems run for months with bad sectors, and this case is no
different. Let the bcache users' IOs succeed if possible but notify them
with pr_crit_once().

A reboot might loose data. They could be lucky with important data in the
page cache; notifying them without killing the device because it is dirty
might give them a chance to do a hot backup before rebooting (or
stop/starting bcache).

Since the dirty map is still in memory, that information is
useful for recovery. After a reboot the dirty map is lost---and with it
the data about what is consistent and what is not.

For example, if LVM snapshots sit atop of the bcache volume, then you
could `dd` them off. If you hit an error, you know that copy is at least
partially inconsistent and can try an older snapshot until one is found
which is old enough to be 100% consistent. Without the dirty map, you
would only be guessing at which volume is actually consistent.

Let users set recovery_io_error=0 for those who really want to fail early.

--
Eric Wheeler