lkml.org 
[lkml]   [2010]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [dm-devel] [PATCH 1/2] block: fix leaks associated with discard request payload
    From
    Date
    On Tue, 2010-06-29 at 18:28 -0400, Mikulas Patocka wrote:
    >
    > On Sun, 27 Jun 2010, James Bottomley wrote:
    >
    > > linux-scsi cc added, since it's a SCSI patch.
    > >
    > > On Sat, 2010-06-26 at 15:56 -0400, Mike Snitzer wrote:
    > > > Fix leaks introduced via "block: don't allocate a payload for discard
    > > > request" commit a1d949f5f44.
    > > >
    > > > sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup
    > > > discard request's payload directly in scsi_finish_command().
    > > >
    > > > Also cleanup page allocated for discard payload in
    > > > scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.
    > > >
    > > > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
    > > > ---
    > > > block/blk-core.c | 23 +++++++++++++++++++++++
    > > > drivers/scsi/scsi.c | 8 ++++++++
    > > > drivers/scsi/sd.c | 18 ++++++++----------
    > > > include/linux/blkdev.h | 1 +
    > > > 4 files changed, 40 insertions(+), 10 deletions(-)
    > > >
    > > > diff --git a/block/blk-core.c b/block/blk-core.c
    > > > index 98b4cee..07925aa 100644
    > > > --- a/block/blk-core.c
    > > > +++ b/block/blk-core.c
    > > > @@ -1167,6 +1167,29 @@ void blk_add_request_payload(struct request *rq, struct page *page,
    > > > }
    > > > EXPORT_SYMBOL_GPL(blk_add_request_payload);
    > > >
    > > > +/**
    > > > + * blk_clear_request_payload - clear a request's payload
    > > > + * @rq: request to update
    > > > + *
    > > > + * The driver needs to take care of freeing the payload itself.
    > > > + */
    > > > +void blk_clear_request_payload(struct request *rq)
    > > > +{
    > > > + struct bio *bio = rq->bio;
    > > > +
    > > > + rq->__data_len = rq->resid_len = 0;
    > > > + rq->nr_phys_segments = 0;
    > > > + rq->buffer = NULL;
    > > > +
    > > > + bio->bi_size = 0;
    > > > + bio->bi_vcnt = 0;
    > > > + bio->bi_phys_segments = 0;
    > > > +
    > > > + bio->bi_io_vec->bv_page = NULL;
    > > > + bio->bi_io_vec->bv_len = 0;
    > > > +}
    > > > +EXPORT_SYMBOL_GPL(blk_clear_request_payload);
    > > > +
    > > > void init_request_from_bio(struct request *req, struct bio *bio)
    > > > {
    > > > req->cpu = bio->bi_comp_cpu;
    > > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
    > > > index ad0ed21..69c7ea4 100644
    > > > --- a/drivers/scsi/scsi.c
    > > > +++ b/drivers/scsi/scsi.c
    > > > @@ -851,6 +851,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
    > > > */
    > > > if (good_bytes == old_good_bytes)
    > > > good_bytes -= scsi_get_resid(cmd);
    > > > + } else if (cmd->request->cmd_flags & REQ_DISCARD) {
    > > > + /*
    > > > + * If this is a discard request that originated from the kernel
    > > > + * we need to free our payload here. Note that we need to check
    > > > + * the request flag as the normal payload rules apply for
    > > > + * pass-through UNMAP / WRITE SAME requests.
    > > > + */
    > > > + __free_page(bio_page(cmd->request->bio));
    > >
    > > This is another layering violation: the page is allocated in the Upper
    > > layer and freed in the mid-layer.
    > >
    > > I really hate these growing contortions for discard. They're a clear
    > > signal that we haven't implemented it right.
    > >
    > > So let's first work out how it should be done. I really like Tomo's
    > > idea of doing discard through the normal REQ_TYPE_FS route, which means
    > > we can control the setup in prep and the tear down in done, all confined
    > > to the ULD.
    > >
    > > Where I think I'm at is partially what Christoph says: The command
    > > transformation belongs in the ULD so that's where the allocation and
    > > deallocation should be, and partly what Tomo says in that we should
    > > eliminate the special case paths.
    > >
    > > The payload vs actual request size should be a red herring if we've got
    > > everything correct: only the ULD cares about the request parameters.
    > > Once we've got everything set up, the mid layer and LLD should only care
    > > about the parameters in the command, so we can confine the size changing
    > > part to the ULD doing the discard.
    > >
    > > Could someone take a stab at this and see if it works without layering
    > > violations or any other problematic signals?
    > >
    > > Thanks,
    > >
    > > James
    >
    > Well, I think that you overestimate the importance of scsi code too much.

    Not, I think, a deadly sin for a SCSI maintainer.

    > There is a layering violation in the code. So what --- you either fix the
    > layering violation or let it be there and grind your teeth on it. But in
    > either case, that layering violation won't affect anyone except scsi
    > developers.

    A layering violation is a signal of bad design wherever it occurs, so
    that wasn't a SCSI centric argument.

    > On the other hand, if you say "because we want to avoid layering violation
    > in SCSI, every issuer of discard request must supply an empty page", you
    > create havoc all over the Linux codebase. md, dm, drbd, xvd, virtio ---
    > whatever you think of, will be allocating a dummy page when constructing
    > a discard request.

    Since I didn't actually say any of that, I suggest you re-read text you
    quoted above. The phrase "The command transformation belongs in the ULD
    so that's where the allocation and deallocation should be" might be a
    relevant one to concentrate on.

    > If the layering violation spans only scsi code, it can be eventually
    > fixed, but this, much worse "layering violation" that will be spanning all
    > block device midlayers, won't ever be fixed.
    >
    > Imagine for example --- a discard request arrivers at a dm-snapshot
    > device. The driver splits it into chunks, remaps each chunk to the
    > physical chunk, submits the requests, the elevator merges adjacent
    > requests and submits fewer bigger requests to the device. Now, if you had
    > to allocate a zeroed page each time you are splitting the request, that
    > would exhaust memory and burn cpu needlessly. You delete a 100MB file? ---
    > fine, allocate a 100MB of zeroed pages.

    This is a straw man: You've tried to portray a position I've never
    taken as mine then attack it ... with what is effectively another bogus
    argument.

    It's not an either/or choice. I've asked the relevant parties to
    combine the approaches and see if a REQ_TYPE_FS path that does the
    allocations in the appropriate place, likely the ULD, produces a good
    design.

    > So I say --- let there be a layering violation in the scsi code, but don't
    > put this problem with a page allocation to all the other bio midlayer
    > developers.

    Thanks for explaining that you have nothing to contribute, I'll make
    sure you're not on my list of relevant parties.

    James




    \
     
     \ /
      Last update: 2010-06-30 01:05    [W:0.043 / U:29.492 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site