On Fri 09-10-15 11:03:30, Nikolay Borisov wrote: > On 10/09/2015 10:37 AM, Hillf Danton wrote: > >>>> @@ -109,8 +109,8 @@ static void ext4_finish_bio(struct bio *bio) > >>>> if (bio->bi_error) > >>>> buffer_io_error(bh); > >>>> } while ((bh = bh->b_this_page) != head); > >>>> - bit_spin_unlock(BH_Uptodate_Lock, &head->b_state); > >>>> local_irq_restore(flags); > >>> > >>> What if it takes 100ms to unlock after IRQ restored? > >> > >> I'm not sure I understand in what direction you are going? Care to > >> elaborate? > >> > > Your change introduces extra time cost the lock waiter has to pay in > > the case that irq happens before the lock is released. > > [CC filesystem and mm people. For reference the thread starts here: > http://thread.gmane.org/gmane.linux.kernel/2056996 ] > > Right, I see what you mean and it's a good point but when doing the > patches I was striving for correctness and starting a discussion, hence > the RFC. In any case I'd personally choose correctness over performance > always ;). > > As I'm not an fs/ext4 expert and have added the relevant parties (please > use reply-all from now on so that the thread is not being cut in the > middle) who will be able to say whether it impact is going to be that > big. I guess in this particular code path worrying about this is prudent > as writeback sounds like a heavily used path. > > Maybe the problem should be approached from a different angle e.g. > drain_all_pages and its reliance on the fact that the IPI will always be > delivered in some finite amount of time? But what if a cpu with disabled > interrupts is waiting on the task issuing the IPI? So I have looked through your patch and also original report (thread starts here: https://lkml.org/lkml/2015/10/8/341) and IMHO one question hasn't been properly answered yet: Who is holding BH_Uptodate_Lock we are spinning on? You have suggested in https://lkml.org/lkml/2015/10/8/464 that it was __block_write_full_page_endio() call but that cannot really be the case. BH_Uptodate_Lock is used only in IO completion handlers - end_buffer_async_read, end_buffer_async_write, ext4_finish_bio. So there really should be some end_io function running on some other CPU which holds BH_Uptodate_Lock for that buffer. BTW: I suppose the filesystem uses 4k blocksize, doesn't it? Honza > >>>> + bit_spin_unlock(BH_Uptodate_Lock, &head->b_state); > >>>> if (!under_io) { > >>>> #ifdef CONFIG_EXT4_FS_ENCRYPTION > >>>> if (ctx) > >>>> -- > >>>> 2.5.0 > >>> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
