From: Boaz Harrosh <bharrosh@panasas.com>
To: Matthew Wilcox <willy@linux.intel.com>
Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,
jgarzik@redhat.com, Matthew Wilcox <matthew@wil.cx>
Subject: Re: [PATCH 1/5] Block: Discard may need to allocate pages
Date: Sun, 05 Apr 2009 15:28:07 +0300 [thread overview]
Message-ID: <49D8A3D7.5070507@panasas.com> (raw)
In-Reply-To: <1238683047-13588-1-git-send-email-willy@linux.intel.com>
On 04/02/2009 05:37 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox <matthew@wil.cx>
>
> SCSI and ATA both need to send data to the device. In order to do this,
> the BIO must be allocated with room for a page to be added, and the bio
> needs to be passed to the discard prep function. We also need to free
> the page attached to the BIO before we free it.
>
> init_request_from_bio() is not currently called from a context which
> forbids sleeping, and to make sure it stays that way (so we don't have
> to use GFP_ATOMIC), add a might_sleep() to it.
>
I understand you have inherited this code, but I think it is a bit of a mess
and you are only adding to the it.
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
> block/blk-barrier.c | 4 +++-
> block/blk-core.c | 4 +++-
> block/ioctl.c | 4 +++-
> drivers/mtd/mtd_blkdevs.c | 2 +-
> include/linux/blkdev.h | 3 ++-
> 5 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-barrier.c b/block/blk-barrier.c
> index f7dae57..82a3035 100644
> --- a/block/blk-barrier.c
> +++ b/block/blk-barrier.c
> @@ -356,6 +356,8 @@ static void blkdev_discard_end_io(struct bio *bio, int err)
> clear_bit(BIO_UPTODATE, &bio->bi_flags);
> }
>
> + if (bio_has_data(bio))
> + __free_page(bio_page(bio));
Page freed which was allocated by the LLD
> bio_put(bio);
OK bio was allocated by user code but shouldn't
> }
>
> @@ -387,7 +389,7 @@ int blkdev_issue_discard(struct block_device *bdev,
> return -EOPNOTSUPP;
>
> while (nr_sects && !ret) {
> - bio = bio_alloc(gfp_mask, 0);
> + bio = bio_alloc(gfp_mask, 1);
blkdev_issue_discard() and blk_ioctl_discard() has half a page
of common (and changing) code, could be done to use a common
helper that sets policy about bio allocation sizes and such.
Just my $0.017
> if (!bio)
> return -ENOMEM;
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 996ed90..7899761 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1095,6 +1095,8 @@ EXPORT_SYMBOL(blk_put_request);
>
> void init_request_from_bio(struct request *req, struct bio *bio)
> {
> + might_sleep();
> +
> req->cpu = bio->bi_comp_cpu;
> req->cmd_type = REQ_TYPE_FS;
>
> @@ -1118,7 +1120,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
> req->cmd_flags |= REQ_DISCARD;
> if (bio_barrier(bio))
> req->cmd_flags |= REQ_SOFTBARRIER;
> - req->q->prepare_discard_fn(req->q, req);
> + req->q->prepare_discard_fn(req->q, req, bio);
Allocation of bio page could be done commonly here.
The prepare_discard_fn() is made to return the needed size. It is not as if we actually
give the driver a choice about the allocation.
So now we allocate the page and free it at the same level.
And we do it only in one place.
Same common code in [PATCH 4/5] and [PATCH 4/5] is done once, here.
> } else if (unlikely(bio_barrier(bio)))
> req->cmd_flags |= (REQ_HARDBARRIER | REQ_NOMERGE);
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 0f22e62..088a9ba 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -145,7 +145,7 @@ static int blk_ioctl_discard(struct block_device *bdev, uint64_t start,
> DECLARE_COMPLETION_ONSTACK(wait);
> struct bio *bio;
>
> - bio = bio_alloc(GFP_KERNEL, 0);
> + bio = bio_alloc(GFP_KERNEL, 1);
This is deja vu, don't you think ;)
> if (!bio)
> return -ENOMEM;
>
> @@ -170,6 +170,8 @@ static int blk_ioctl_discard(struct block_device *bdev, uint64_t start,
> ret = -EOPNOTSUPP;
> else if (!bio_flagged(bio, BIO_UPTODATE))
> ret = -EIO;
> + if (bio_has_data(bio))
> + __free_page(bio_page(bio));
> bio_put(bio);
> }
> return ret;
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 1409f01..2b6ed4b 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -33,7 +33,7 @@ struct mtd_blkcore_priv {
> };
>
> static int blktrans_discard_request(struct request_queue *q,
> - struct request *req)
> + struct request *req, struct bio *bio)
> {
> req->cmd_type = REQ_TYPE_LINUX_BLOCK;
> req->cmd[0] = REQ_LB_OP_DISCARD;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 465d6ba..9d9bd7b 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -260,7 +260,8 @@ typedef void (request_fn_proc) (struct request_queue *q);
> typedef int (make_request_fn) (struct request_queue *q, struct bio *bio);
> typedef int (prep_rq_fn) (struct request_queue *, struct request *);
> typedef void (unplug_fn) (struct request_queue *);
> -typedef int (prepare_discard_fn) (struct request_queue *, struct request *);
> +typedef int (prepare_discard_fn) (struct request_queue *, struct request *,
> + struct bio *bio);
>
> struct bio_vec;
> struct bvec_merge_data {
I have one question:
At [PATCH 4/5] and [PATCH 4/5] you do:
+ struct page *page = alloc_page(GFP_KERNEL);
does that zero the alloced page? since if I understand correctly this page
will go on the wire, a SW target on the other size could snoop random Kernel
memory, is that allowed? OK I might be totally clueless here.
Have a good day
Boaz
next prev parent reply other threads:[~2009-04-05 12:30 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-04-02 14:37 [PATCH 1/5] Block: Discard may need to allocate pages Matthew Wilcox
2009-04-02 14:37 ` [PATCH 2/5] Make DISCARD_BARRIER and DISCARD_NOBARRIER writes instead of reads Matthew Wilcox
2009-04-02 14:37 ` [PATCH 3/5] ata: Add TRIM infrastructure Matthew Wilcox
2009-04-02 14:37 ` [PATCH 4/5] ide: Add support for TRIM Matthew Wilcox
2009-04-02 14:37 ` [PATCH 5/5] libata: " Matthew Wilcox
2009-04-02 17:20 ` Mark Lord
2009-04-02 17:55 ` Matthew Wilcox
2009-04-16 20:25 ` Mark Lord
2009-04-17 19:44 ` Mark Lord
2009-04-02 15:58 ` [PATCH 4/5] ide: " Sergei Shtylyov
2009-04-02 16:28 ` Matthew Wilcox
2009-04-02 16:38 ` Sergei Shtylyov
2009-04-02 16:51 ` Matthew Wilcox
2009-04-02 19:37 ` Bartlomiej Zolnierkiewicz
2009-04-07 21:38 ` Bartlomiej Zolnierkiewicz
2009-04-07 22:15 ` Matthew Wilcox
2009-04-07 22:26 ` Jeff Garzik
2009-04-07 22:35 ` Bartlomiej Zolnierkiewicz
2009-04-07 17:20 ` Jeff Garzik
2009-04-07 17:57 ` Mark Lord
2009-04-07 18:10 ` Markus Trippelsdorf
2009-04-07 19:58 ` Mark Lord
2009-04-08 7:14 ` Markus Trippelsdorf
2009-04-08 14:25 ` Mark Lord
2009-04-08 14:33 ` Mark Lord
2009-04-08 14:44 ` Dongjun Shin
2009-04-08 14:59 ` Jeff Garzik
2009-04-08 15:50 ` Mark Lord
2009-04-02 15:55 ` [PATCH 3/5] ata: Add TRIM infrastructure Sergei Shtylyov
2009-04-02 16:18 ` Matthew Wilcox
2009-04-02 16:32 ` Sergei Shtylyov
2009-04-02 16:47 ` Matthew Wilcox
2009-04-07 0:02 ` Jeff Garzik
2009-04-05 12:28 ` Boaz Harrosh [this message]
2009-04-06 20:34 ` [PATCH 1/5] Block: Discard may need to allocate pages Matthew Wilcox
2009-05-03 6:11 ` Matthew Wilcox
2009-05-03 7:16 ` New TRIM/UNMAP tree published (2009-05-02) Matthew Wilcox
2009-05-03 13:07 ` Hugh Dickins
2009-05-03 14:48 ` Matthew Wilcox
2009-05-03 15:02 ` Boaz Harrosh
2009-05-03 15:42 ` Matthew Wilcox
2009-05-03 16:34 ` Boaz Harrosh
2009-05-03 18:34 ` Jeff Garzik
2009-05-03 18:40 ` Jeff Garzik
2009-05-03 19:04 ` James Bottomley
2009-05-03 19:20 ` Jeff Garzik
2009-05-03 19:37 ` James Bottomley
2009-05-04 14:03 ` Douglas Gilbert
2009-05-04 14:40 ` James Bottomley
2009-05-04 15:11 ` Douglas Gilbert
2009-05-04 15:23 ` James Bottomley
2009-05-03 19:47 ` James Bottomley
2009-05-03 22:47 ` Jeff Garzik
2009-05-04 15:28 ` Boaz Harrosh
2009-05-03 21:48 ` Matthew Wilcox
2009-05-03 22:54 ` Jeff Garzik
2009-05-03 18:48 ` Bartlomiej Zolnierkiewicz
2009-05-03 15:05 ` Hugh Dickins
2009-04-17 21:23 ` [PATCH 1/5] Block: Discard may need to allocate pages Mark Lord
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=49D8A3D7.5070507@panasas.com \
--to=bharrosh@panasas.com \
--cc=jgarzik@redhat.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew@wil.cx \
--cc=willy@linux.intel.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).