while nthreads malloc fails, return err value directory
without kfree.
And the same issue arises in nfs_lookup, so return res
directly without freeing fh and fattr.
Signed-off-by: Rui Xiang <[email protected]>
---
fs/nfs/dir.c | 4 ++--
fs/nfsd/nfsctl.c | 7 ++++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 812154a..2518865 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1278,7 +1278,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
res = ERR_PTR(-ENAMETOOLONG);
if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
- goto out;
+ return res;
/*
* If we're doing an exclusive create, optimize away the lookup
@@ -1287,7 +1287,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
if (nfs_is_exclusive_create(dir, flags)) {
d_instantiate(dentry, NULL);
res = NULL;
- goto out;
+ return res;
}
res = ERR_PTR(-ENOMEM);
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 7f55517..3c5e748 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -491,9 +491,10 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
}
nthreads = kcalloc(npools, sizeof(int), GFP_KERNEL);
- rv = -ENOMEM;
- if (nthreads == NULL)
- goto out_free;
+ if (nthreads == NULL) {
+ mutex_unlock(&nfsd_mutex);
+ return -ENOENT;
+ }
if (size > 0) {
for (i = 0; i < npools; i++) {
--
1.8.2.2
In common function nfs_instantiate to create, mkdir, and mknod,
if dentry->d_inode exits, it should return -EEXIST instead of
-EACCES.
Signed-off-by: Rui Xiang <[email protected]>
---
fs/nfs/dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2518865..e570b37 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1547,7 +1547,7 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
struct dentry *parent = dget_parent(dentry);
struct inode *dir = parent->d_inode;
struct inode *inode;
- int error = -EACCES;
+ int error = -EEXIST;
d_drop(dentry);
--
1.8.2.2
On 2013/12/11 0:26, Trond Myklebust wrote:
> On Mon, 2013-12-09 at 18:42 +0800, Rui Xiang wrote:
>> On 2013/12/6 21:46, Trond Myklebust wrote:
>>>
>>> On Dec 6, 2013, at 4:30, Rui Xiang <[email protected]> wrote:
>>>
>>>> In common function nfs_instantiate to create, mkdir, and mknod,
>>>> if dentry->d_inode exits, it should return -EEXIST instead of
>>>> -EACCES.
>>>>
>>>> Signed-off-by: Rui Xiang <[email protected]>
>>>> ---
>>>> fs/nfs/dir.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>>> index 2518865..e570b37 100644
>>>> --- a/fs/nfs/dir.c
>>>> +++ b/fs/nfs/dir.c
>>>> @@ -1547,7 +1547,7 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
>>>> struct dentry *parent = dget_parent(dentry);
>>>> struct inode *dir = parent->d_inode;
>>>> struct inode *inode;
>>>> - int error = -EACCES;
>>>> + int error = -EEXIST;
>>>>
>>>> d_drop(dentry);
>>>>
>>>
>>> That looks like it should rather be a WARN_ON(). If the caller has set the dentry's inode before creating the file, then something is really wrong.
>> It can return -EEXIST to say that the file already exits, then will exit. But why does it need a WARN_ON(). Please give me some advise.
>>
>
> If we ever hit that condition then it means that the caller is doing
> something very wrong. That's what the WARN_ON (or WARN_ON_ONCE) would be
> there to check.
>
OK. In that case, is it necessary to send a new patch with the WARN_ON?
Thanks
Rui
On 2013/12/6 17:30, Rui Xiang wrote:
> while nthreads malloc fails, return err value directory
> without kfree.
>
> And the same issue arises in nfs_lookup, so return res
> directly without freeing fh and fattr.
>
> Signed-off-by: Rui Xiang <[email protected]>
> ---
> fs/nfs/dir.c | 4 ++--
> fs/nfsd/nfsctl.c | 7 ++++---
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 812154a..2518865 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1278,7 +1278,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
>
> res = ERR_PTR(-ENAMETOOLONG);
> if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
> - goto out;
> + return res;
>
> /*
> * If we're doing an exclusive create, optimize away the lookup
> @@ -1287,7 +1287,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
> if (nfs_is_exclusive_create(dir, flags)) {
> d_instantiate(dentry, NULL);
> res = NULL;
> - goto out;
> + return res;
> }
>
> res = ERR_PTR(-ENOMEM);
Trond, and what's your opinion on the chunk above?
Thanks
Rui
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7f55517..3c5e748 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -491,9 +491,10 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
> }
>
> nthreads = kcalloc(npools, sizeof(int), GFP_KERNEL);
> - rv = -ENOMEM;
> - if (nthreads == NULL)
> - goto out_free;
> + if (nthreads == NULL) {
> + mutex_unlock(&nfsd_mutex);
> + return -ENOENT;
> + }
>
> if (size > 0) {
> for (i = 0; i < npools; i++) {
>
On Dec 6, 2013, at 4:30, Rui Xiang <[email protected]> wrote:
> In common function nfs_instantiate to create, mkdir, and mknod,
> if dentry->d_inode exits, it should return -EEXIST instead of
> -EACCES.
>
> Signed-off-by: Rui Xiang <[email protected]>
> ---
> fs/nfs/dir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 2518865..e570b37 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1547,7 +1547,7 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
> struct dentry *parent = dget_parent(dentry);
> struct inode *dir = parent->d_inode;
> struct inode *inode;
> - int error = -EACCES;
> + int error = -EEXIST;
>
> d_drop(sentry);
>
That looks like it should rather be a WARN_ON(). If the caller has set the dentry's inode before creating the file, then something is really wrong.
Cheers
Trond
On Mon, 2013-12-09 at 18:42 +0800, Rui Xiang wrote:
> On 2013/12/6 21:46, Trond Myklebust wrote:
> >
> > On Dec 6, 2013, at 4:30, Rui Xiang <[email protected]> wrote:
> >
> >> In common function nfs_instantiate to create, mkdir, and mknod,
> >> if dentry->d_inode exits, it should return -EEXIST instead of
> >> -EACCES.
> >>
> >> Signed-off-by: Rui Xiang <[email protected]>
> >> ---
> >> fs/nfs/dir.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> >> index 2518865..e570b37 100644
> >> --- a/fs/nfs/dir.c
> >> +++ b/fs/nfs/dir.c
> >> @@ -1547,7 +1547,7 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
> >> struct dentry *parent = dget_parent(dentry);
> >> struct inode *dir = parent->d_inode;
> >> struct inode *inode;
> >> - int error = -EACCES;
> >> + int error = -EEXIST;
> >>
> >> d_drop(dentry);
> >>
> >
> > That looks like it should rather be a WARN_ON(). If the caller has set the dentry's inode before creating the file, then something is really wrong.
> It can return -EEXIST to say that the file already exits, then will exit. But why does it need a WARN_ON(). Please give me some advise.
>
If we ever hit that condition then it means that the caller is doing
something very wrong. That's what the WARN_ON (or WARN_ON_ONCE) would be
there to check.
Cheers
Trond
On Fri, Dec 06, 2013 at 05:30:16PM +0800, Rui Xiang wrote:
> while nthreads malloc fails, return err value directory
> without kfree.
>
> And the same issue arises in nfs_lookup, so return res
> directly without freeing fh and fattr.
>
> Signed-off-by: Rui Xiang <[email protected]>
> ---
> fs/nfs/dir.c | 4 ++--
> fs/nfsd/nfsctl.c | 7 ++++---
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 812154a..2518865 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1278,7 +1278,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
>
> res = ERR_PTR(-ENAMETOOLONG);
> if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
> - goto out;
> + return res;
>
> /*
> * If we're doing an exclusive create, optimize away the lookup
> @@ -1287,7 +1287,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
> if (nfs_is_exclusive_create(dir, flags)) {
> d_instantiate(dentry, NULL);
> res = NULL;
> - goto out;
> + return res;
> }
>
> res = ERR_PTR(-ENOMEM);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7f55517..3c5e748 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -491,9 +491,10 @@ static ssize_t write_pool_threads(struct file *file, char *buf, size_t size)
> }
>
> nthreads = kcalloc(npools, sizeof(int), GFP_KERNEL);
> - rv = -ENOMEM;
> - if (nthreads == NULL)
> - goto out_free;
> + if (nthreads == NULL) {
> + mutex_unlock(&nfsd_mutex);
> + return -ENOENT;
> + }
>
> if (size > 0) {
> for (i = 0; i < npools; i++) {
> --
Looks fine. Could you send this last chunk to me as a separate patch?
--b.
> 1.8.2.2
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-12-09 at 20:33 +0800, Rui Xiang wrote:
> On 2013/12/6 17:30, Rui Xiang wrote:
> > while nthreads malloc fails, return err value directory
> > without kfree.
> >
> > And the same issue arises in nfs_lookup, so return res
> > directly without freeing fh and fattr.
> >
> > Signed-off-by: Rui Xiang <[email protected]>
> > ---
> > fs/nfs/dir.c | 4 ++--
> > fs/nfsd/nfsctl.c | 7 ++++---
> > 2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 812154a..2518865 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -1278,7 +1278,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
> >
> > res = ERR_PTR(-ENAMETOOLONG);
> > if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
> > - goto out;
> > + return res;
> >
> > /*
> > * If we're doing an exclusive create, optimize away the lookup
> > @@ -1287,7 +1287,7 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
> > if (nfs_is_exclusive_create(dir, flags)) {
> > d_instantiate(dentry, NULL);
> > res = NULL;
> > - goto out;
> > + return res;
> > }
> >
> > res = ERR_PTR(-ENOMEM);
> Trond, and what's your opinion on the chunk above?
It's a minor optimisation but certainly not something that needs to be
pushed in before the next merge window.
Cheers
Trond
On 2013/12/6 21:46, Trond Myklebust wrote:
>
> On Dec 6, 2013, at 4:30, Rui Xiang <[email protected]> wrote:
>
>> In common function nfs_instantiate to create, mkdir, and mknod,
>> if dentry->d_inode exits, it should return -EEXIST instead of
>> -EACCES.
>>
>> Signed-off-by: Rui Xiang <[email protected]>
>> ---
>> fs/nfs/dir.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>> index 2518865..e570b37 100644
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -1547,7 +1547,7 @@ int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
>> struct dentry *parent = dget_parent(dentry);
>> struct inode *dir = parent->d_inode;
>> struct inode *inode;
>> - int error = -EACCES;
>> + int error = -EEXIST;
>>
>> d_drop(dentry);
>>
>
> That looks like it should rather be a WARN_ON(). If the caller has set the dentry's inode before creating the file, then something is really wrong.
It can return -EEXIST to say that the file already exits, then will exit. But why does it need a WARN_ON(). Please give me some advise.
Thanks
Rui
>
> Cheers
> Trond
>