All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
To: linux-bcache-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
	Jens Axboe <axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>,
	Lars Ellenberg
	<lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org>,
	Hannes Reinecke <hare-l3A5Bk7waGM@public.gmane.org>
Subject: Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing
Date: Mon, 17 Sep 2012 13:51:39 -0700	[thread overview]
Message-ID: <20120917205139.GB14492@google.com> (raw)
In-Reply-To: <20120914182828.GK15728-FDJ95KluN3Z0klwcnFlA1dvLeJWuRmrY@public.gmane.org>

On Fri, Sep 14, 2012 at 07:28:28PM +0100, Alasdair G Kergon wrote:
> On Thu, Sep 06, 2012 at 03:34:55PM -0700, Kent Overstreet wrote:
> > With the old code, when you allocate a bio from a bio pool you have to
> > implement your own destructor that knows how to find the bio pool the
> > bio was originally allocated from.
> > 
> > This adds a new field to struct bio (bi_pool) and changes
> > bio_alloc_bioset() to use it. This makes various bio destructors
> > unnecessary, so they're then deleted.
> > 
> > v6: Explain the temporary if statement in bio_put
>  
> This patch also silently reverts 
> commit 4d7b38b7d944a79da3793b6c92d38682f3905ac9
> "dm: clear bi_end_io on remapping failure"
> 
> Why?
> 
> If it's intentional, please explain it in your patch header and
> copy Hannes to reconsider the matter.

Never noticed that was introduced in its own patch until you pointed it
out.

That isn't a very good patch - it says it's clearing bi_end_io as a
precaution, but as a precaution to what?

As far as I can tell, it was never necessary. The bio is about to be
freed - there shouldn't be any other references on it (__bio_map() is
called on freshly allocated bios, and bio_get() is never called in dm.c)
Nothing else should've been looking at bi_end_io, certainly the
destructor didn't.

Now that there's no destructor, it makes even less sense to have it -
after that bio_put() that bio isn't being touched by dm code anymore.

WARNING: multiple messages have this Message-ID (diff)
From: Kent Overstreet <koverstreet@google.com>
To: linux-bcache@vger.kernel.org, linux-kernel@vger.kernel.org,
	dm-devel@redhat.com, Jens Axboe <axboe@kernel.dk>,
	Lars Ellenberg <lars.ellenberg@linbit.com>,
	Hannes Reinecke <hare@suse.de>
Subject: Re: [dm-devel] [PATCH v10 1/8] block: Generalized bio pool freeing
Date: Mon, 17 Sep 2012 13:51:39 -0700	[thread overview]
Message-ID: <20120917205139.GB14492@google.com> (raw)
In-Reply-To: <20120914182828.GK15728@agk-dp.fab.redhat.com>

On Fri, Sep 14, 2012 at 07:28:28PM +0100, Alasdair G Kergon wrote:
> On Thu, Sep 06, 2012 at 03:34:55PM -0700, Kent Overstreet wrote:
> > With the old code, when you allocate a bio from a bio pool you have to
> > implement your own destructor that knows how to find the bio pool the
> > bio was originally allocated from.
> > 
> > This adds a new field to struct bio (bi_pool) and changes
> > bio_alloc_bioset() to use it. This makes various bio destructors
> > unnecessary, so they're then deleted.
> > 
> > v6: Explain the temporary if statement in bio_put
>  
> This patch also silently reverts 
> commit 4d7b38b7d944a79da3793b6c92d38682f3905ac9
> "dm: clear bi_end_io on remapping failure"
> 
> Why?
> 
> If it's intentional, please explain it in your patch header and
> copy Hannes to reconsider the matter.

Never noticed that was introduced in its own patch until you pointed it
out.

That isn't a very good patch - it says it's clearing bi_end_io as a
precaution, but as a precaution to what?

As far as I can tell, it was never necessary. The bio is about to be
freed - there shouldn't be any other references on it (__bio_map() is
called on freshly allocated bios, and bio_get() is never called in dm.c)
Nothing else should've been looking at bi_end_io, certainly the
destructor didn't.

Now that there's no destructor, it makes even less sense to have it -
after that bio_put() that bio isn't being touched by dm code anymore.

  parent reply	other threads:[~2012-09-17 20:51 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-06 22:34 [PATCH v10 0/8] Block cleanups Kent Overstreet
2012-09-06 22:34 ` Kent Overstreet
2012-09-06 22:34 ` [PATCH v10 1/8] block: Generalized bio pool freeing Kent Overstreet
2012-09-06 22:34   ` Kent Overstreet
     [not found]   ` <1346970902-10931-2-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-14 18:28     ` [dm-devel] " Alasdair G Kergon
2012-09-14 18:28       ` Alasdair G Kergon
     [not found]       ` <20120914182828.GK15728-FDJ95KluN3Z0klwcnFlA1dvLeJWuRmrY@public.gmane.org>
2012-09-17 20:51         ` Kent Overstreet [this message]
2012-09-17 20:51           ` Kent Overstreet
     [not found]           ` <20120917205139.GB14492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-18 16:31             ` Alasdair G Kergon
2012-09-18 16:31               ` Alasdair G Kergon
2012-09-14 18:36     ` Alasdair G Kergon
2012-09-14 18:36       ` Alasdair G Kergon
2012-09-06 22:34 ` [PATCH v10 2/8] block: Ues bi_pool for bio_integrity_alloc() Kent Overstreet
2012-09-06 22:34   ` Kent Overstreet
2012-09-06 22:34 ` [PATCH v10 3/8] dm: Use bioset's front_pad for dm_rq_clone_bio_info Kent Overstreet
2012-09-06 22:34 ` [PATCH v10 4/8] block: Add bio_reset() Kent Overstreet
     [not found]   ` <1346970902-10931-5-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-07  1:34     ` Jens Axboe
2012-09-07  1:34       ` Jens Axboe
2012-09-07 20:58       ` Kent Overstreet
     [not found]         ` <20120907205823.GD16360-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-07 21:55           ` Jens Axboe
2012-09-07 21:55             ` Jens Axboe
     [not found]             ` <504A6D57.1030607-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
2012-09-07 22:06               ` Jens Axboe
2012-09-07 22:06                 ` Jens Axboe
     [not found]                 ` <504A6FF5.3090603-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
2012-09-07 22:25                   ` Kent Overstreet
2012-09-07 22:25                     ` Kent Overstreet
     [not found]                     ` <20120907222522.GE16360-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-07 22:44                       ` Jens Axboe
2012-09-07 22:44                         ` Jens Axboe
     [not found]                         ` <504A78B0.8010105-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
2012-09-07 23:14                           ` [dm-devel] " Alasdair G Kergon
2012-09-07 23:14                             ` Alasdair G Kergon
     [not found]                             ` <20120907231433.GE10309-FDJ95KluN3Z0klwcnFlA1dvLeJWuRmrY@public.gmane.org>
2012-09-07 23:25                               ` Kent Overstreet
2012-09-07 23:25                                 ` Kent Overstreet
2012-09-06 22:34 ` [PATCH v10 5/8] pktcdvd: Switch to bio_kmalloc() Kent Overstreet
2012-09-06 22:35 ` [PATCH v10 6/8] block: Kill bi_destructor Kent Overstreet
     [not found]   ` <1346970902-10931-7-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-06 23:42     ` Tejun Heo
2012-09-06 23:42       ` Tejun Heo
2012-09-06 22:35 ` [PATCH v10 7/8] block: Consolidate bio_alloc_bioset(), bio_kmalloc() Kent Overstreet
     [not found]   ` <1346970902-10931-8-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-06 23:45     ` Tejun Heo
2012-09-06 23:45       ` Tejun Heo
2012-09-06 22:35 ` [PATCH v10 8/8] block: Add bio_clone_bioset(), bio_clone_kmalloc() Kent Overstreet
     [not found]   ` <1346970902-10931-9-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-06 23:46     ` Tejun Heo
2012-09-06 23:46       ` Tejun Heo
2012-09-14 21:50     ` [dm-devel] " Alasdair G Kergon
2012-09-14 21:50       ` Alasdair G Kergon
     [not found]       ` <20120914215059.GQ15728-FDJ95KluN3Z0klwcnFlA1dvLeJWuRmrY@public.gmane.org>
2012-09-17 20:42         ` Kent Overstreet
2012-09-17 20:42           ` Kent Overstreet
     [not found]           ` <20120917204227.GA14492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-18 16:15             ` Alasdair G Kergon
2012-09-18 16:15               ` Alasdair G Kergon
     [not found] ` <1346970902-10931-1-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-06 23:48   ` [PATCH v10 0/8] Block cleanups Tejun Heo
2012-09-06 23:48     ` Tejun Heo
     [not found]     ` <20120906234805.GD9426-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2012-09-07  1:37       ` Jens Axboe
2012-09-07  1:37         ` Jens Axboe
2012-09-07 20:44         ` Kent Overstreet
2012-09-11  4:43       ` NeilBrown
2012-09-11  4:43         ` NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120917205139.GB14492@google.com \
    --to=koverstreet-hpiqsd4aklfqt0dzr+alfa@public.gmane.org \
    --cc=axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org \
    --cc=dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=hare-l3A5Bk7waGM@public.gmane.org \
    --cc=lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org \
    --cc=linux-bcache-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.