Eric Wheeler <bcache@xxxxxxxxxxxxxxxxxx> writes:
> [+cc Mikulas Patocka, Jeff Moyer; Do either of you have any input on Lars'
> commentary related to patchwork #'s 9204125 and 7398411 and BZ#119841? ]
Sorry, I don't have any time to look at this right now.
Cheers,
Jeff
>
> On Tue, 19 Jul 2016, Lars Ellenberg wrote:
>
>> On Tue, Jul 12, 2016 at 10:32:33PM -0400, Mike Snitzer wrote:
>> > On Tue, Jul 12 2016 at 10:18pm -0400,
>> > Eric Wheeler <bcache@xxxxxxxxxxxxxxxxxx> wrote:
>> >
>> > > On Tue, 12 Jul 2016, NeilBrown wrote:
>> > >
>> > > > On Tue, Jul 12 2016, Lars Ellenberg wrote:
>> > > > ....
>> > > > >
>> > > > > Instead, I suggest to distinguish between recursive calls to
>> > > > > generic_make_request(), and pushing back the remainder part in
>> > > > > blk_queue_split(), by pointing current->bio_lists to a
>> > > > > struct recursion_to_iteration_bio_lists {
>> > > > > struct bio_list recursion;
>> > > > > struct bio_list queue;
>> > > > > }
>> > > > >
>> > > > > By providing each q->make_request_fn() with an empty "recursion"
>> > > > > bio_list, then merging any recursively submitted bios to the
>> > > > > head of the "queue" list, we can make the recursion-to-iteration
>> > > > > logic in generic_make_request() process deepest level bios first,
>> > > > > and "sibling" bios of the same level in "natural" order.
>> > > > >
>> > > > > Signed-off-by: Lars Ellenberg <lars.ellenberg@xxxxxxxxxx>
>> > > > > Signed-off-by: Roland Kammerer <roland.kammerer@xxxxxxxxxx>
>> > > >
>> > > > Reviewed-by: NeilBrown <neilb@xxxxxxxx>
>> > > >
>> > > > Thanks again for doing this - I think this is a very significant
>> > > > improvement and could allow other simplifications.
>> > >
>> > > Thank you Lars for all of this work!
>> > >
>> > > It seems like there have been many 4.3+ blockdev stacking issues and this
>> > > will certainly address some of those (maybe all of them?). (I think we
>> > > hit this while trying drbd in 4.4 so we dropped back to 4.1 without
>> > > issue.) It would be great to hear 4.4.y stable pick this up if
>> > > compatible.
>> > >
>> > >
>> > > Do you believe that this patch would solve any of the proposals by others
>> > > since 4.3 related to bio splitting/large bios? I've been collecting a
>> > > list, none of which appear have landed yet as of 4.7-rc7 (but correct me
>> > > if I'm wrong):
>> > >
>> > > A. [PATCH v2] block: make sure big bio is splitted into at most 256 bvecs
>> > > by Ming Lei: https://patchwork.kernel.org/patch/9169483/
>>
>> That's an independend issue.
>>
>> > > B. block: don't make BLK_DEF_MAX_SECTORS too big
>> > > by Shaohua Li: http://www.spinics.net/lists/linux-bcache/msg03525.html
>>
>> Yet an other independend issue.
>>
>> > > C. [1/3] block: flush queued bios when process blocks to avoid deadlock
>> > > by Mikulas Patocka: https://patchwork.kernel.org/patch/9204125/
>> > > (was https://patchwork.kernel.org/patch/7398411/)
>>
>> As it stands now,
>> this is yet an other issue, but related.
>>
>> From the link above:
>>
>> | ** Here is the dm-snapshot deadlock that was observed:
>> |
>> | 1) Process A sends one-page read bio to the dm-snapshot target. The bio
>> | spans snapshot chunk boundary and so it is split to two bios by device
>> | mapper.
>> |
>> | 2) Device mapper creates the first sub-bio and sends it to the snapshot
>> | driver.
>> |
>> | 3) The function snapshot_map calls track_chunk (that allocates a
>> | structure
>> | dm_snap_tracked_chunk and adds it to tracked_chunk_hash) and then remaps
>> | the bio to the underlying device and exits with DM_MAPIO_REMAPPED.
>> |
>> | 4) The remapped bio is submitted with generic_make_request, but it isn't
>> | issued - it is added to current->bio_list instead.
>> |
>> | 5) Meanwhile, process B (dm's kcopyd) executes pending_complete for the
>> | chunk affected be the first remapped bio, it takes down_write(&s->lock)
>> | and then loops in __check_for_conflicting_io, waiting for
>> | dm_snap_tracked_chunk created in step 3) to be released.
>> |
>> | 6) Process A continues, it creates a second sub-bio for the rest of the
>> | original bio.
>>
>> Aha.
>> Here is the relation.
>> If "A" had only ever processed "just the chunk it can handle now",
>> and "pushed back" the rest of the incoming bio,
>> it could rely on all deeper level bios to have been submitted already.
>>
>> But this does not look like it easily fits into the current DM model.
>>
>> | 7) snapshot_map is called for this new bio, it waits on
>> | down_write(&s->lock) that is held by Process B (in step 5).
>>
>> There is an other suggestion:
>> Use down_trylock (or down_timeout),
>> and if it fails, push back the currently to-be-processed bio.
>> We can introduce a new bio helper for that.
>> Kind of what blk_queue_split() does with my patch applied.
>>
>> Or even better, ignore the down_trylock suggestion,
>> simply not iterate over all pieces first,
>> but process one piece, and return back the the
>> iteration in generic_make_request.
>>
>> A bit of conflict here may be that DM has all its own
>> split and clone and queue magic, and wants to process
>> "all of the bio" before returning back to generic_make_request().
>>
>> To change that, __split_and_process_bio() and all its helpers
>> would need to learn to "push back" (pieces of) the bio they are
>> currently working on, and not push back via "DM_ENDIO_REQUEUE",
>> but by bio_list_add_head(¤t->bio_lists->queue, piece_to_be_done_later).
>>
>> Then, after they processed each piece,
>> *return* all the way up to the top-level generic_make_request(),
>> where the recursion-to-iteration logic would then
>> make sure that all deeper level bios, submitted via
>> recursive calls to generic_make_request() will be processed, before the
>> next, pushed back, piece of the "original incoming" bio.
>>
>> And *not* do their own iteration over all pieces first.
>>
>> Probably not as easy as dropping the while loop,
>> using bio_advance, and pushing that "advanced" bio back to
>> current->...queue?
>>
>> static void __split_and_process_bio(struct mapped_device *md,
>> struct dm_table *map, struct bio *bio)
>> ...
>> ci.bio = bio;
>> ci.sector_count = bio_sectors(bio);
>> while (ci.sector_count && !error)
>> error = __split_and_process_non_flush(&ci);
>> ...
>> error = __split_and_process_non_flush(&ci);
>> if (ci.sector_count)
>> bio_advance()
>> bio_list_add_head(¤t->bio_lists->queue, )
>> ...
>>
>> Something like that, maybe?
>> Just a thought.
>>
>> > > D. dm-crypt: Fix error with too large bios
>> > > by Mikulas Patocka: https://patchwork.kernel.org/patch/9138595/
>> > >
>> > > The A,B,D are known to fix large bio issues when stacking dm+bcache
>> > > (though the B,D are trivial and probably necessary even with your patch).
>> > >
>> > > Patch C was mentioned earlier in this thread by Mike Snitzer and you
>> > > commented briefly that his patch might solve the issue; given that, and in
>> > > the interest of minimizing duplicate effort, which of the following best
>> > > describes the situation?
>> > >
>> > > 1. Your patch could supersede Mikulas's patch; they address the same
>> > > issue.
>> > >
>> > > 2. Mikulas's patch addresses different issues such and both patches
>> > > should be applied.
>> > >
>> > > 3. There is overlap between both your patch and Mikulas's such that both
>> > > #1,#2 are true and effort to solve this has been duplicated.
>> > >
>> > >
>> > > If #3, then what might be done to resolve the overlap?
>> >
>> > Mikulas confirmed to me that he believes Lars' v2 patch will fix the
>> > dm-snapshot problem, which is being tracked with this BZ:
>> > https://bugzilla.kernel.org/show_bug.cgi?id=119841
>> >
>> > We'll see how testing goes (currently underway).
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
