lkml.org 
[lkml]   [2010]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 1/2] blkdev: fix merge_bvec_fn return value checks
    Date
    Mike Snitzer <snitzer@redhat.com> writes:

    > On Tue, Mar 2, 2010 at 10:49 PM, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
    >> Jens Axboe <jens.axboe@oracle.com> writes:
    >>
    >>> On Sat, Feb 27 2010, Dmitry Monakhov wrote:
    >>>> merge_bvec_fn() returns bvec->bv_len on success. So we have to check
    >>>> against this value. But in case of fs_optimization merge we compare
    >>>> with wrong value. This patch must be included in
    >>>>  b428cd6da7e6559aca69aa2e3a526037d3f20403
    >>>> But accidentally i've forgot to add this in the initial patch.
    >>>> To make things straight let's replace all such checks.
    >>>> In fact this makes code easy to understand.
    >>>
    >>> Agree, applied.
    >> Ohh.. as you already know this patch break dm-layer. Sorry.
    >> This is because dm->merge may return more than requested. So correct
    >> check must test against less what requested. Correct patch attached.
    >
    > Yes, it is quite common for dm_merge_bvec() to return greater than the
    > requested length.
    >
    > But dm_merge_bvec() returning a maximum length, rather than requested,
    > isn't special. All the other blk_queue_merge_bvec() callers' merge
    > functions appear to return "maximum amount of bytes we can accept at
    > this offset" too.
    What for? Does it allow us to make some optimization?
    For example like follows:
    bio_add_pageS(bio, **pages) {
    /* call merge_fn only one untill all space exhausted */
    ret = merge_fn() (this returns huge value (1024*1024))
    while (ret) {
    bio->bi_io_vec[bio->bi_vcnt - 1].bv_page = page;
    ...
    ret -= PAGE_SIZE;
    bio->bi_vcnt++;
    }
    }
    IMHO the answer is *NO*, this code will unlikely to work.
    >
    > __bio_add_page() only needs to care about whether the
    > 'q->merge_bvec_fn' return is _less than_ the requested length.
    >
    >> From 145fb49bf2251f445ca29c5218333367448932d6 Mon Sep 17 00:00:00 2001
    >> From: Dmitry Monakhov <dmonakhov@openvz.org>
    >> Date: Wed, 3 Mar 2010 06:28:06 +0300
    >> Subject: [PATCH] blkdev: fix merge_bvec_fn return value checks v2
    >>
    >> merge_bvec_fn() returns bvec->bv_len on success. So we have to check
    >> against this value. But in case of fs_optimization merge we compare
    >> with wrong value. This patch must be included in
    >>  b428cd6da7e6559aca69aa2e3a526037d3f20403
    >> But accidentally i've forgot to add this in the initial patch.
    >> To make things straight let's replace all such checks.
    >> In fact this makes code easy to understand.
    >>
    >> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
    >> ---
    >>  fs/bio.c |    4 ++--
    >>  1 files changed, 2 insertions(+), 2 deletions(-)
    >>
    >> diff --git a/fs/bio.c b/fs/bio.c
    >> index 88094af..975657a 100644
    >> --- a/fs/bio.c
    >> +++ b/fs/bio.c
    >> @@ -557,7 +557,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page
    >>                                        .bi_rw = bio->bi_rw,
    >>                                };
    >>
    >> -                               if (q->merge_bvec_fn(q, &bvm, prev) < len) {
    >> +                               if (q->merge_bvec_fn(q, &bvm, prev) < prev->bv_len) {
    >>                                        prev->bv_len -= len;
    >>                                        return 0;
    >>                                }
    >> @@ -611,7 +611,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct page
    >>                 * merge_bvec_fn() returns number of bytes it can accept
    >>                 * at this offset
    >>                 */
    >> -               if (q->merge_bvec_fn(q, &bvm, bvec) < len) {
    >> +               if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
    >>                        bvec->bv_page = NULL;
    >>                        bvec->bv_len = 0;
    >>                        bvec->bv_offset = 0;
    >
    > NOTE this 2nd hunk doesn't change anything at all because: bvec->bv_len = len;
    Yess. IMHO this makes code more readable.
    >
    > Mike
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-03-03 19:47    [W:0.032 / U:59.656 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site