Message ID | 1608694025-121050-1-git-send-email-yejune.deng@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | io_uring: remove io_remove_personalities() | expand |
On Wed, Dec 23, 2020 at 11:27:05AM +0800, Yejune Deng wrote: >The function io_remove_personalities() is very similar to >io_unregister_personality(),but the latter has a more reasonable >return value. > >Signed-off-by: Yejune Deng <yejune.deng@gmail.com> >--- > fs/io_uring.c | 25 ++++++------------------- > 1 file changed, 6 insertions(+), 19 deletions(-) The patch LGTM, maybe as an alternative you can leave io_remove_personality() with the interface needed by idr_for_each() and implement io_unregister_personality() calling io_remove_personality() with the right parameters. Just an idea, but I'm also fine with this patch, so: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/fs/io_uring.c b/fs/io_uring.c >index b749578..000ea9a 100644 >--- a/fs/io_uring.c >+++ b/fs/io_uring.c >@@ -8608,7 +8608,7 @@ static int io_uring_fasync(int fd, struct file *file, int on) > return fasync_helper(fd, file, on, &ctx->cq_fasync); > } > >-static int io_remove_personalities(int id, void *p, void *data) >+static int io_unregister_personality(int id, void *p, void *data) > { > struct io_ring_ctx *ctx = data; > struct io_identity *iod; >@@ -8618,8 +8618,10 @@ static int io_remove_personalities(int id, void *p, void *data) > put_cred(iod->creds); > if (refcount_dec_and_test(&iod->count)) > kfree(iod); >+ return 0; > } >- return 0; >+ >+ return -EINVAL; > } > > static void io_ring_exit_work(struct work_struct *work) >@@ -8657,7 +8659,7 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx) > > /* if we failed setting up the ctx, we might not have any rings */ > io_iopoll_try_reap_events(ctx); >- idr_for_each(&ctx->personality_idr, io_remove_personalities, ctx); >+ idr_for_each(&ctx->personality_idr, io_unregister_personality, ctx); > > /* > * Do this upfront, so we won't have a grace period where the ring >@@ -9679,21 +9681,6 @@ static int io_register_personality(struct io_ring_ctx *ctx) > return ret; > } > >-static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id) >-{ >- struct io_identity *iod; >- >- iod = idr_remove(&ctx->personality_idr, id); >- if (iod) { >- put_cred(iod->creds); >- if (refcount_dec_and_test(&iod->count)) >- kfree(iod); >- return 0; >- } >- >- return -EINVAL; >-} >- > static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg, > unsigned int nr_args) > { >@@ -9906,7 +9893,7 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode, > ret = -EINVAL; > if (arg) > break; >- ret = io_unregister_personality(ctx, nr_args); >+ ret = io_unregister_personality(nr_args, NULL, ctx); > break; > case IORING_REGISTER_ENABLE_RINGS: > ret = -EINVAL; >-- >1.9.1 >
On 23/12/2020 10:36, Stefano Garzarella wrote: > On Wed, Dec 23, 2020 at 11:27:05AM +0800, Yejune Deng wrote: >> The function io_remove_personalities() is very similar to >> io_unregister_personality(),but the latter has a more reasonable >> return value. >> >> Signed-off-by: Yejune Deng <yejune.deng@gmail.com> >> --- >> fs/io_uring.c | 25 ++++++------------------- >> 1 file changed, 6 insertions(+), 19 deletions(-) > > The patch LGTM, maybe as an alternative you can leave io_remove_personality() with the interface needed by idr_for_each() and implement io_unregister_personality() calling io_remove_personality() with the right parameters. Right, don't replace sane types with void * just because. Leave well-typed io_unregister_personality() and call it from io_remove_personalities(). Also * idr_for_each() - Iterate through all stored pointers. ... * If @fn returns anything other than %0, the iteration stops and that * value is returned from this function. For io_remove_personality() iod==NULL should not happen because it's under for_each and synchronised, but leave the return value be io_remove_personality(void *, ...) { struct io_ring_ctx *ctx = data; io_unregister_personality(ctx, id); return 0; }
OK,I will adopt it and resubmit. On Wed, Dec 23, 2020 at 8:45 PM Pavel Begunkov <asml.silence@gmail.com> wrote: > > On 23/12/2020 10:36, Stefano Garzarella wrote: > > On Wed, Dec 23, 2020 at 11:27:05AM +0800, Yejune Deng wrote: > >> The function io_remove_personalities() is very similar to > >> io_unregister_personality(),but the latter has a more reasonable > >> return value. > >> > >> Signed-off-by: Yejune Deng <yejune.deng@gmail.com> > >> --- > >> fs/io_uring.c | 25 ++++++------------------- > >> 1 file changed, 6 insertions(+), 19 deletions(-) > > > > The patch LGTM, maybe as an alternative you can leave io_remove_personality() with the interface needed by idr_for_each() and implement io_unregister_personality() calling io_remove_personality() with the right parameters. > > Right, don't replace sane types with void * just because. > Leave well-typed io_unregister_personality() and call it from > io_remove_personalities(). > > > Also > * idr_for_each() - Iterate through all stored pointers. > ... > * If @fn returns anything other than %0, the iteration stops and that > * value is returned from this function. > > For io_remove_personality() iod==NULL should not happen because > it's under for_each and synchronised, but leave the return value be > > io_remove_personality(void *, ...) > { > struct io_ring_ctx *ctx = data; > > io_unregister_personality(ctx, id); > return 0; > } > > -- > Pavel Begunkov
diff --git a/fs/io_uring.c b/fs/io_uring.c index b749578..000ea9a 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8608,7 +8608,7 @@ static int io_uring_fasync(int fd, struct file *file, int on) return fasync_helper(fd, file, on, &ctx->cq_fasync); } -static int io_remove_personalities(int id, void *p, void *data) +static int io_unregister_personality(int id, void *p, void *data) { struct io_ring_ctx *ctx = data; struct io_identity *iod; @@ -8618,8 +8618,10 @@ static int io_remove_personalities(int id, void *p, void *data) put_cred(iod->creds); if (refcount_dec_and_test(&iod->count)) kfree(iod); + return 0; } - return 0; + + return -EINVAL; } static void io_ring_exit_work(struct work_struct *work) @@ -8657,7 +8659,7 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx) /* if we failed setting up the ctx, we might not have any rings */ io_iopoll_try_reap_events(ctx); - idr_for_each(&ctx->personality_idr, io_remove_personalities, ctx); + idr_for_each(&ctx->personality_idr, io_unregister_personality, ctx); /* * Do this upfront, so we won't have a grace period where the ring @@ -9679,21 +9681,6 @@ static int io_register_personality(struct io_ring_ctx *ctx) return ret; } -static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id) -{ - struct io_identity *iod; - - iod = idr_remove(&ctx->personality_idr, id); - if (iod) { - put_cred(iod->creds); - if (refcount_dec_and_test(&iod->count)) - kfree(iod); - return 0; - } - - return -EINVAL; -} - static int io_register_restrictions(struct io_ring_ctx *ctx, void __user *arg, unsigned int nr_args) { @@ -9906,7 +9893,7 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode, ret = -EINVAL; if (arg) break; - ret = io_unregister_personality(ctx, nr_args); + ret = io_unregister_personality(nr_args, NULL, ctx); break; case IORING_REGISTER_ENABLE_RINGS: ret = -EINVAL;
The function io_remove_personalities() is very similar to io_unregister_personality(),but the latter has a more reasonable return value. Signed-off-by: Yejune Deng <yejune.deng@gmail.com> --- fs/io_uring.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)