Re: [PATCH] mm/gup: Use put_user_page*() instead of put_page*()
On 7/14/19 1:08 PM, Bharath Vedartham wrote:
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 4ef62a4..b4a4549 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2694,10 +2694,9 @@ static int io_sqe_buffer_register(struct io_ring_ctx
> *ctx, void __user *arg,
>* if we did partial map, or found file backed vmas,
>* release any pages we did get
>*/
> - if (pret > 0) {
> - for (j = 0; j < pret; j++)
> - put_page(pages[j]);
> - }
> + if (pret > 0)
> + put_user_pages(pages, pret);
> +
> if (ctx->account_mem)
> io_unaccount_mem(ctx->user, nr_pages);
> kvfree(imu->bvec);
You handled just the failure case of the buffer registration, but not
the actual free in io_sqe_buffer_unregister().
--
Jens Axboe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments
On 08/31/2017 05:29 PM, Kees Cook wrote: > Several timer users needlessly reset their .function/.data fields during > their timer callback, but nothing else changes them. Some users do not > use their .data field at all. Each instance is removed here. For amiflop: Acked-by: Jens Axboe -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: replace IS_ERR and PTR_ERR with PTR_ERR_OR_ZERO
On 11/06/2013 12:56 AM, Duan Jiong wrote: > This patch fixes coccinelle error regarding usage of IS_ERR and > PTR_ERR instead of PTR_ERR_OR_ZERO. Applied, thanks. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: replace IS_ERR and PTR_ERR with PTR_ERR_OR_ZERO
On 11/06/2013 12:55 AM, Duan Jiong wrote: > This patch fixes coccinelle error regarding usage of IS_ERR and > PTR_ERR instead of PTR_ERR_OR_ZERO. Applied, thanks. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On 2014-06-06 11:52, James Bottomley wrote:
On Fri, 2014-06-06 at 12:18 -0500, Mike Christie wrote:
On 6/5/14, 9:53 PM, KY Srinivasan wrote:
-Original Message-
From: Mike Christie [mailto:micha...@cs.wisc.edu]
Sent: Thursday, June 5, 2014 6:33 PM
To: KY Srinivasan
Cc: James Bottomley; linux-ker...@vger.kernel.org; a...@canonical.com;
de...@linuxdriverproject.org; h...@infradead.org; linux-
s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
jasow...@redhat.com
Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
from the basic I/O timeout
On 06/04/2014 12:15 PM, KY Srinivasan wrote:
-Original Message-
From: James Bottomley [mailto:jbottom...@parallels.com]
Sent: Wednesday, June 4, 2014 10:02 AM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; a...@canonical.com;
de...@linuxdriverproject.org; h...@infradead.org; linux-
s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
jasow...@redhat.com
Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the
FLUSH_TIMEOUT from the basic I/O timeout
On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to
derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this
patch did not use the basic I/O timeout of the device. Fix this bug.
Signed-off-by: K. Y. Srinivasan
---
drivers/scsi/sd.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
e9689d5..54150b1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct
scsi_device *sdp, struct request *rq)
static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
request *rq) {
- rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
+ int timeout = sdp->request_queue->rq_timeout;
+
+ rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
Could you share where you found this to be a problem? It looks like
a bug in block because all inbound requests being prepared should
have a timeout set, so block would be the place to fix it.
Perhaps; what I found was that the value in rq->timeout was 0 coming
into this function and thus multiplying obviously has no effect.
I think you are right. We hit this problem because we are doing:
scsi_request_fn -> blk_peek_request -> sd_prep_fn ->
scsi_setup_flush_cmnd.
At this time request->timeout is zero so the multiplication does nothing. See
how sd_setup_write_same_cmnd will set the request->timeout at this time.
Then in scsi_request_fn we do:
scsi_request_fn -> blk_start_request -> blk_add_timer.
At this time it will set the request->timeout if something like req block pc
users (like scsi_execute() or block/scsi_ioctl.c) or the write same code
mentioned above have not set the timeout.
I don't think this is a recent change. Prior to this commit, we were setting
the timeout
value in this function; it just happened to be a different constant unrelated
to the I/O
timeout.
Yeah, it looks like when 7e660100d85af860e7ad763202fff717adcdaacd was
merged we were supposed to initialize it like in your patch in this thread.
I guess we could do your patch in this thread, or if we want the block
layer to initialize the timeout before the prep_fn callout is called
then we would need to have the blk-flush.c code to that when it sets up
the request. If we do the latter, do we want the discard and write same
code to initialize the request's timeout before the prep_fn callout is
called too?
I looked through the call chain; it seems to be intentional behaviour on
the part of block. Just from an mq point of view, it would make better
code if we unconditionally initialised rq->timeout early and allowed
prep to modify it and then dumped the if(!req->timeout) in
blk_add_timer(), but it's a marginal if condition that would compile to
a conditional store on sensible architectures, so losing the conditional
probably isn't worth worrying about.
Cc'd Jens for his opinion with the block patch
I just committed this one earlier today:
http://git.kernel.dk/?p=linux-block.git;a=commit;h=f6be4fb4bcb396fc3b1c134b7863351972de081f
since I ran into the same thing on nvme. Either approach is fine with
me, as they both allow override of the timeout before insertion. But
we've always done the rq->timeout = 0 init, so I think we should just
reinstate that behavior.
--
Jens Axboe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 2:15 AM, Allen Pais wrote: > From: Allen Pais > > In preparation for unconditionally passing the > struct tasklet_struct pointer to all tasklet > callbacks, switch to using the new tasklet_setup() > and from_tasklet() to pass the tasklet pointer explicitly. Who came up with the idea to add a macro 'from_tasklet' that is just container_of? container_of in the code would be _much_ more readable, and not leave anyone guessing wtf from_tasklet is doing. I'd fix that up now before everything else goes in... -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 12:29 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >> On 8/17/20 2:15 AM, Allen Pais wrote: >>> From: Allen Pais >>> >>> In preparation for unconditionally passing the >>> struct tasklet_struct pointer to all tasklet >>> callbacks, switch to using the new tasklet_setup() >>> and from_tasklet() to pass the tasklet pointer explicitly. >> >> Who came up with the idea to add a macro 'from_tasklet' that is just >> container_of? container_of in the code would be _much_ more readable, >> and not leave anyone guessing wtf from_tasklet is doing. >> >> I'd fix that up now before everything else goes in... > > As I mentioned in the other thread, I think this makes things much more > readable. It's the same thing that the timer_struct conversion did > (added a container_of wrapper) to avoid the ever-repeating use of > typeof(), long lines, etc. But then it should use a generic name, instead of each sub-system using some random name that makes people look up exactly what it does. I'm not huge fan of the container_of() redundancy, but adding private variants of this doesn't seem like the best way forward. Let's have a generic helper that does this, and use it everywhere. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/17/20 12:48 PM, Kees Cook wrote: > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >> On 8/17/20 12:29 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>> From: Allen Pais >>>>> >>>>> In preparation for unconditionally passing the >>>>> struct tasklet_struct pointer to all tasklet >>>>> callbacks, switch to using the new tasklet_setup() >>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>> >>>> Who came up with the idea to add a macro 'from_tasklet' that is just >>>> container_of? container_of in the code would be _much_ more readable, >>>> and not leave anyone guessing wtf from_tasklet is doing. >>>> >>>> I'd fix that up now before everything else goes in... >>> >>> As I mentioned in the other thread, I think this makes things much more >>> readable. It's the same thing that the timer_struct conversion did >>> (added a container_of wrapper) to avoid the ever-repeating use of >>> typeof(), long lines, etc. >> >> But then it should use a generic name, instead of each sub-system using >> some random name that makes people look up exactly what it does. I'm not >> huge fan of the container_of() redundancy, but adding private variants >> of this doesn't seem like the best way forward. Let's have a generic >> helper that does this, and use it everywhere. > > I'm open to suggestions, but as things stand, these kinds of treewide On naming? Implementation is just as it stands, from_tasklet() is totally generic which is why I objected to it. from_member()? Not great with naming... But I can see this going further and then we'll suddenly have tons of these. It's not good for readability. > changes end up getting whole-release delays because of the need to have > the API in place for everyone before patches to do the changes can be > sent to multiple maintainers, etc. Sure, that's always true of treewide changes like that. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/18/20 1:00 PM, James Bottomley wrote: > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >> On 8/17/20 12:48 PM, Kees Cook wrote: >>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>> From: Allen Pais >>>>>>> >>>>>>> In preparation for unconditionally passing the >>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>> >>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>> is just container_of? container_of in the code would be >>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>> from_tasklet is doing. >>>>>> >>>>>> I'd fix that up now before everything else goes in... >>>>> >>>>> As I mentioned in the other thread, I think this makes things >>>>> much more readable. It's the same thing that the timer_struct >>>>> conversion did (added a container_of wrapper) to avoid the >>>>> ever-repeating use of typeof(), long lines, etc. >>>> >>>> But then it should use a generic name, instead of each sub-system >>>> using some random name that makes people look up exactly what it >>>> does. I'm not huge fan of the container_of() redundancy, but >>>> adding private variants of this doesn't seem like the best way >>>> forward. Let's have a generic helper that does this, and use it >>>> everywhere. >>> >>> I'm open to suggestions, but as things stand, these kinds of >>> treewide >> >> On naming? Implementation is just as it stands, from_tasklet() is >> totally generic which is why I objected to it. from_member()? Not >> great with naming... But I can see this going further and then we'll >> suddenly have tons of these. It's not good for readability. > > Since both threads seem to have petered out, let me suggest in > kernel.h: > > #define cast_out(ptr, container, member) \ > container_of(ptr, typeof(*container), member) > > It does what you want, the argument order is the same as container_of > with the only difference being you name the containing structure > instead of having to specify its type. Not to incessantly bike shed on the naming, but I don't like cast_out, it's not very descriptive. And it has connotations of getting rid of something, which isn't really true. FWIW, I like the from_ part of the original naming, as it has some clues as to what is being done here. Why not just from_container()? That should immediately tell people what it does without having to look up the implementation, even before this becomes a part of the accepted coding norm. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/19/20 6:11 AM, Greg KH wrote: > On Wed, Aug 19, 2020 at 07:00:53AM -0600, Jens Axboe wrote: >> On 8/18/20 1:00 PM, James Bottomley wrote: >>> On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: >>>> On 8/17/20 12:48 PM, Kees Cook wrote: >>>>> On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: >>>>>> On 8/17/20 12:29 PM, Kees Cook wrote: >>>>>>> On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: >>>>>>>> On 8/17/20 2:15 AM, Allen Pais wrote: >>>>>>>>> From: Allen Pais >>>>>>>>> >>>>>>>>> In preparation for unconditionally passing the >>>>>>>>> struct tasklet_struct pointer to all tasklet >>>>>>>>> callbacks, switch to using the new tasklet_setup() >>>>>>>>> and from_tasklet() to pass the tasklet pointer explicitly. >>>>>>>> >>>>>>>> Who came up with the idea to add a macro 'from_tasklet' that >>>>>>>> is just container_of? container_of in the code would be >>>>>>>> _much_ more readable, and not leave anyone guessing wtf >>>>>>>> from_tasklet is doing. >>>>>>>> >>>>>>>> I'd fix that up now before everything else goes in... >>>>>>> >>>>>>> As I mentioned in the other thread, I think this makes things >>>>>>> much more readable. It's the same thing that the timer_struct >>>>>>> conversion did (added a container_of wrapper) to avoid the >>>>>>> ever-repeating use of typeof(), long lines, etc. >>>>>> >>>>>> But then it should use a generic name, instead of each sub-system >>>>>> using some random name that makes people look up exactly what it >>>>>> does. I'm not huge fan of the container_of() redundancy, but >>>>>> adding private variants of this doesn't seem like the best way >>>>>> forward. Let's have a generic helper that does this, and use it >>>>>> everywhere. >>>>> >>>>> I'm open to suggestions, but as things stand, these kinds of >>>>> treewide >>>> >>>> On naming? Implementation is just as it stands, from_tasklet() is >>>> totally generic which is why I objected to it. from_member()? Not >>>> great with naming... But I can see this going further and then we'll >>>> suddenly have tons of these. It's not good for readability. >>> >>> Since both threads seem to have petered out, let me suggest in >>> kernel.h: >>> >>> #define cast_out(ptr, container, member) \ >>> container_of(ptr, typeof(*container), member) >>> >>> It does what you want, the argument order is the same as container_of >>> with the only difference being you name the containing structure >>> instead of having to specify its type. >> >> Not to incessantly bike shed on the naming, but I don't like cast_out, >> it's not very descriptive. And it has connotations of getting rid of >> something, which isn't really true. > > I agree, if we want to bike shed, I don't like this color either. > >> FWIW, I like the from_ part of the original naming, as it has some clues >> as to what is being done here. Why not just from_container()? That >> should immediately tell people what it does without having to look up >> the implementation, even before this becomes a part of the accepted >> coding norm. > > Why are people hating on the well-known and used container_of()? > > If you really hate to type the type and want a new macro, what about > 'container_from()'? (noun/verb is nicer to sort symbols by...) > > But really, why is this even needed? container_from() or from_container(), either works just fine for me in terms of naming. I think people are hating on it because it makes for _really_ long lines, and it's arguably cleaner/simpler to just pass in the pointer type instead. Then you end up with lines like this: struct request_queue *q = container_of(work, struct request_queue, requeue_work.work); But I'm not the one that started this addition of from_tasklet(), my objection was adding a private macro for something that should be generic functionality. Hence I think we either need to provide that, or tell the from_tasklet() folks that they should just use container_of(). -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On 8/19/20 9:24 AM, Allen wrote: >> [...] >>>> Since both threads seem to have petered out, let me suggest in >>>> kernel.h: >>>> >>>> #define cast_out(ptr, container, member) \ >>>> container_of(ptr, typeof(*container), member) >>>> >>>> It does what you want, the argument order is the same as >>>> container_of with the only difference being you name the containing >>>> structure instead of having to specify its type. >>> >>> Not to incessantly bike shed on the naming, but I don't like >>> cast_out, it's not very descriptive. And it has connotations of >>> getting rid of something, which isn't really true. >> >> Um, I thought it was exactly descriptive: you're casting to the outer >> container. I thought about following the C++ dynamic casting style, so >> out_cast(), but that seemed a bit pejorative. What about outer_cast()? >> >>> FWIW, I like the from_ part of the original naming, as it has some >>> clues as to what is being done here. Why not just from_container()? >>> That should immediately tell people what it does without having to >>> look up the implementation, even before this becomes a part of the >>> accepted coding norm. >> >> I'm not opposed to container_from() but it seems a little less >> descriptive than outer_cast() but I don't really care. I always have >> to look up container_of() when I'm using it so this would just be >> another macro of that type ... >> > > So far we have a few which have been suggested as replacement > for from_tasklet() > > - out_cast() or outer_cast() > - from_member(). > - container_from() or from_container() > > from_container() sounds fine, would trimming it a bit work? like from_cont(). I like container_from() the most, since it's the closest to contain_of() which is a well known idiom for years. The lines will already be shorter without the need to specify the struct, so don't like the idea of squeezing container into cont for any of them. For most people, cont is usually short for continue, not container. -- Jens Axboe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
