Page MenuHomeFreeBSD

ffs_backgroundwritedone: Set INVAL on errored IOs to avoid bdirty
AbandonedPublic

Authored by cse_cem_gmail_com on Jun 24 2015, 5:04 PM.

Details

Reviewers
kib
Summary

It is invalid to dirty an uncached buf, so do not. This avoids a
scenario where we would attempt to dirty a buf with NULL bufobj,
resulting in an ASSERT (INVARIANTS kernel) or NULL dereference
(non-DEBUG kernel).

Test Plan
Repro:

db> show mount
0xfffff80007833330 /dev/vtbd0p2 on / (ufs)

...
$ kgdb
...
(gdb) p ((struct ufsmount*)((struct mount*)0xfffff80007833330).mnt_data).um_devvp
$5 = (struct vnode *) 0xfffff80007846ce8

...
$ sysctl 'debug.fail_point.ffs_backgroundwritedone_error=return(0x07846ce8)'
$ sync

(Before): Panic in bdirty -> reassignbuf
(After): No panic, log shows "ffs_backgroundwritedone: Erroring IO ..."

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

cse_cem_gmail_com retitled this revision from to ffs_backgroundwritedone: Set INVAL on errored IOs to avoid bdirty.
cse_cem_gmail_com updated this object.
cse_cem_gmail_com edited the test plan for this revision. (Show Details)
cse_cem_gmail_com added a reviewer: kib.
cse_cem_gmail_com added subscribers: benno, bdrewery, markj.

I would add a comment explaining the reason for B_INVAL there. Isn't there a bigger problem with the BIO_ERROR ? The real cylinder group buffer is marked clean in ffs_bufwrite(), which means that we lost the update, making the filesystem inconsistent.

For testing, Isilon contributed the KFAIL thing, which is mostly unused. Look at the buf_vm_page_count_severe how it was intended to introduce the sporadic faults at the selected points.

In D2900#56463, @kib wrote:

I would add a comment explaining the reason for B_INVAL there.

Ok.

Isn't there a bigger problem with the BIO_ERROR ? The real cylinder group buffer is marked clean in ffs_bufwrite(), which means that we lost the update, making the filesystem inconsistent.

Yep. In our case CAM kicked out the backing device on error so the filesystem is completely gone. I don't know if CAM does this on vanilla FreeBSD.

(ada1:ata3:0:1:0): WRITE_DMA. ACB: ca 00 c0 a8 92 40 00 00 00 00 20 00
(ada1:ata3:0:1:0): CAM status: ATA Status Error
(ada1:ata3:0:1:0): ATA status: 51 (DRDY SERV ERR), error: 10 (IDNF )
(ada1:ata3:0:1:0): RES: 51 10 c0 a8 92 00 00 00 00 20 00
(ada1:ata3:0:1:0): Retrying command
In D2900#56463, @kib wrote:

For testing, Isilon contributed the KFAIL thing, which is mostly unused. Look at the buf_vm_page_count_severe how it was intended to introduce the sporadic faults at the selected points.

Sure, it's easy enough to add something like this. It seems like we could vgone() the devvp here to match the scenario I am seeing. I am gating it on the devvp for now, but this is somewhat hacky. if you have a better suggestion, please let me know.

cse_cem_gmail_com edited the test plan for this revision. (Show Details)
cse_cem_gmail_com edited edge metadata.

Address kib's feedback; add B_INVAL comment, add failpoint.

We cannot vgone() the devvp at this point because we can't sleep on the devvp
vnode lock in g_up thread.

Landed, with Konstantin's additional work (and Kirk's blessing) to avoid *CORRUPTION*, in r284887.