>>>>> "Robert" == Robert LeBlanc <robert@xxxxxxxxxxxxx> writes: Robert> On Thu, Dec 1, 2016 at 9:04 AM, John Stoffel <john@xxxxxxxxxxx> wrote: >> Maybe this is pedantic, but why not have an 'else' instead of the >> return to make it more clear? If someone (like me :-) missed the >> return on the quick read through, it's going to be some confusion. >> >> if (bio_data_dir(bio) == READ) >> raid1_read_request(mddev, bui, r1_bio); >> else >> raid1_write_request(mddev, bio, r1_bio, start_next_window); >> >> seems cleaner. Robert> Yeah, I agree that is a bit cleaner, I was just following the Robert> previous style. I'll change it to what you propose. It's not a big deal, whatever makes you happy. >> Also, why are the calls different with the start_next_window parameter >> added in? Sorry for being dense, trying to understand the code >> better... Robert> start_next_windows is set by wait_barrier() and is only Robert> non-zero for writes and the return value of zero is never used Robert> for reads. I tried to move that code into the write branch, Robert> but I could not unload the module due to conf->nr_pending == Robert> conf->queued+extra failing in wait_event_lock_irq_cmd() in Robert> freeze_array(). Each read was decrementing conf->nr_pending Robert> and since wait_barrier() was not being called on reads, Robert> conf->nr_pending was very negative and would not return to Robert> zero. In order to refactor that out completely would require a Robert> lot more work and changes. I'd need to do more research into Robert> how much the spin_locks are needed for reading when there is Robert> recovery going on before I could start to try and refactor Robert> that. The other option is to break wait_barrier() into two Robert> functions, one that gets called for both read and write and Robert> the other for just the writes. I can do the latter pretty Robert> easily if that is a desired direction. That second option might be the way to move forward. But it sounds like a bigger refactoring than is really needed now. John -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
