On Thu, Aug 20, 2020 at 2:06 AM Xiao Ni <xni@xxxxxxxxxx> wrote:
[...]
> >> We need to clear blocked_rdev before this goto, or put retry_wait label
> >> before "blocked_rdev = NULL;". I guess this path is not tested...
> > I did test for this patch with mkfs with/without resync and wrote some
> > files to device.
> > And ran fstrim after writing some files. The patch worked well during
> > the test.
> > For blocked device situation, I didn't test. I'll add this test.
> >>
> >> We are duplicating a lot of logic from raid10_write_request() here.
> >> Can we
> >> try to pull the common logic into a helper function?
> > I'll do this.
>
> Hi Song
>
> At first I had thought about this. In raid10_write_request, it checks
> blocked rdev, badblock and sets
> r10_bio->devs[].bio in one loop. Can we move the codes that check
> blocked rdev into a separate loop,
> similar like this:
>
> wait_block:
> for (i = 0; i < conf->copies; i++) {
> /* check rdev/rrdev is block or not
> * If it's block, goto wait_block
> */
> }
>
> for (i = 0; i < conf->copies; i++) {
> /* check bad block
> * sets r10_bio->devs[i].bio
> */
> }
>
> This way it waits until all rdev devices are not blocked. There is a
> possibility that some rdev devices change
> to blocked again after checking. But the way raid10_write_request uses
> now has this risk too. If you think
> the method mentioned above is OK, I'll try to use this way.
I think it is possible to make the code work this way. Let's try refactor
raid10_write_request() like this. If it doesn't work for some reason, we
can revisit this part.
Thanks,
Song
