Fix: segmentation fault of write throttle while import

[GeLiXin]

The issue
Async writes may be issued before the pool finish the process of initialization, which means we may access a NULL point of spa->spa_dsl_pool in function vdev_queue_max_async_writes illegality. typically this happens when the self-healing zio was issued by mirror while importing.

The analysis
Pool always imported with flag of FWRITE | FREAD, when we open the meta objset with ZIO error in one of our multi-DVAS(Both silent and “noisy” data corruption), the logical vdev which have mirror ops will try to use the good data it have in hand to repair damaged children. When the self-healing write zio issues to the leaf vdev, it encount the write throttle strategy which demand the statistics of dirty data in spa->spa_dsl_pool->dp_dirty_total. Unfortunately, the pointer of spa->spa_dsl_pool will keep NULL until we open the meta objset success, and then, segmentation fault comes up.

The patch
Do not ask for the dirty data statistics anymore in vdev_queue_max_async_writes before the pool pointer get initialized. Instead we push data out as fast as possible to speed up the self-healing process triggered by import.

[GeLiXin]

What I commit seems not relevant to the failed build, and test. Have I left out something?

[behlendorf]

@GeLiXin nice find and analysis. Your proposed fix LGTM.

Another possible fix for this would be to check spl->spa_dsl_pool in vdev_mirror_io_done() right after the spa_writeable() check and then never dispatch the repair IO. A similar change would need to be made in vdev_raidz_io_done(). Of course this has the significant downside of not repairing the pool immediately. So I personally prefer your approach even if the pool isn't 100% initialized but it would be great to get @grwilson's thoughts on this.

• Please rebase you work on to the latest master branch and force update this PR. This will resolve the build failures you observed with the latest Linux kernels (they move fast!).
• Include your excellent description of the issue and analysis in the commit comment. You don't need to include the stack traces, we'll just link back to this issue.
• [style] use the spa_get_dsl() accessor function to reference spa->spa_dsl_pool. The original code didn't do this but it should have and we can make that fix now.
• [style] remove the parenthesis around the single line conditional you added.

[GeLiXin]

@behlendorf thanks for your advice, I have modified the code according to your proposal.

I agree with you. In my opinion, repairing the data is doing the right thing, since we will never damage the good data under the protection of checksum. The other thing remains a concern is whether we do it at the right time. In consideration of the MOS data is significant enough to keep it fit as soon as possible, and just a little data will be issued from dsl_pool_init (after the dsl_pool_init, write throttle can work normally), even if it's not a perfect time, it's can't be a bad time. So I just let the self-healing zio get through the leaf vdev, instead of make more checks to prevent them from dispatch.

[grwilson]

@GeLiXin out of curiosity do you know what I/O is being repaired? We call dsl_pool_init early in spa_load_impl to make sure that spa_dsl_pool is initialized. Prior to this point we should only be reading the label so we must have inadvertently introduced a data block read prior to dsl_pool_init and I would like to make sure that we address this (if possible).

[GeLiXin]

@grwilson I'm very glad to get your attention. ZFS is such an Amazing filesystem from which I am learning a lot, thank you for your contribution to ZFS which give me more chance to learn.

@behlendorf Heartfelt thanks to you, for your professional proposal and meticulous review.

@grwilson Fortunately I think I can address the read IO who request the repair. I used to have a pool result in coredump every time I try to import it, and every time the coredump take place, there are two stacks as below(no other pools running):

Thread 126 (Thread 4542):
#0 0x00007f5008fa016c in pthread_cond_wait@@GLIBC_2.3.2 () from /lib/libpthread.so.0
#1 0x00007f500e069bae in cv_wait () from /version/lib/lib_naslfs_dll.so
#2 0x00007f500e19ab53 in zio_wait () from /version/lib/lib_naslfs_dll.so
#3 0x00007f500e1218b0 in arc_read() from /version/lib/lib_naslfs_dll.so
#4 0x00007f500e12fda9 in dmu_objset_open_impl () from /version/lib/lib_naslfs_dll.so
#5 0x00007f500e150762 in dsl_pool_init () from /version/lib/lib_naslfs_dll.so
#6 0x00007f500e16570f in spa_load_impl.clone.10 () from /version/lib/lib_naslfs_dll.so
#7 0x00007f500e1664bf in spa_load () from /version/lib/lib_naslfs_dll.so
#8 0x00007f500e166c77 in spa_load_best () from /version/lib/lib_naslfs_dll.so
#9 0x00007f500e167724 in spa_import () from /version/lib/lib_naslfs_dll.so

Thread 115 (Thread 4531):
#0 0x00007f5008fa3cd7 in do_sigwait () from /lib/libpthread.so.0
#1 0x00007f5008fa3d57 in sigwait () from /lib/libpthread.so.0
#2 0x00007f5009532d2c in Vos_SuspendTask () from /version/lib/lib_com_dll.so
#3 signal handler called
#4 0x00007f500e182a0e in vdev_queue_io_to_issue () from /version/lib/lib_naslfs_dll.so
#5 0x00007f500e1833a6 in vdev_queue_io () from /version/lib/lib_naslfs_dll.so
#6 0x00007f500e19c315 in zio_vdev_io_start () from /version/lib/lib_naslfs_dll.so
#7 0x00007f500e197cea in zio_execute () from /version/lib/lib_naslfs_dll.so
#8 0x00007f500e181914 in vdev_mirror_io_done () from /version/lib/lib_naslfs_dll.so
#9 0x00007f500e198475 in zio_vdev_io_done () from /version/lib/lib_naslfs_dll.so
#10 0x00007f500e197cea in zio_execute () from /version/lib/lib_naslfs_dll.so
#11 0x00007f500e19ca93 in zio_done () from /version/lib/lib_naslfs_dll.so
#12 0x00007f500e197cea in zio_execute () from /version/lib/lib_naslfs_dll.so
#13 0x00007f500e070f79 in taskq_thread () from /version/lib/lib_naslfs_dll.so

I find that dsl_pool_init is the only one who request a read at that moment, and the write IO must be a repair IO because it issued from the vdev_mirror_io_done. So we do not inadvertently introduced a data block read prior to dsl_pool_init, it's just the dsl_pool_init itself who dispatch the read. One step further, after we get the ub_rootbp from the label, we read the objset_phys_t of our meta objset which pointed by ub_rootbp, this is the very point we encounter this issue, not the point prior to dsl_pool_init.

[grwilson]

@GeLiXin Thanks for providing this information, it was extremely helpful. I was looking at the code in dsl_pool_init and it seems that it might be more appropriate to make a change here to address this issue. I say this because the goal is to have spa_dsl_pool available as early in load time as possible. So something like the following might be more suitable:

int
dsl_pool_init(spa_t *spa, uint64_t txg, dsl_pool_t **dpp)
{
        int err;
        dsl_pool_t *dp = dsl_pool_open_impl(spa, txg);
        *dpp = dp;

        err = dmu_objset_open_impl(spa, NULL, &dp->dp_meta_rootbp,
            &dp->dp_meta_objset);
        if (err != 0) {
                dsl_pool_close(dp);
                *dpp = NULL;
        }

        return (err);
}

[behlendorf]

@grwilson thanks for taking the time to look at this and propose a fix which gets to the heart of the matter.

I like the approach but I'm a little concerned that your fix subtly depends on the caller of dsl_pool_init() passing in the &spa->spa_dsl_pool. This happens to be true, but it not obvious that passing in a different dsl_pool_t ** would reintroduce this bug.

So why don't we both. @GeLiXin could you add another commit with @grwilson's patch, properly attributed as the author, this PR.

[GeLiXin]

@grwilson thanks for your essential angle of view, it makes this patch more impeccable.

@behlendorf a new commit was added in this PR according to @grwilson's proposal now.

[behlendorf]

@GeLiXin thanks, LGTM.