diff mbox

ceph: checking for IS_ERR instead of NULL

Message ID 20160126092444.GB15717@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Jan. 26, 2016, 9:24 a.m. UTC 
ceph_osdc_alloc_request() returns NULL on error, it never returns error
pointers.

Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ilya Dryomov Jan. 26, 2016, 10:30 a.m. UTC | #1 
On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> ceph_osdc_alloc_request() returns NULL on error, it never returns error
> pointers.
>
> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index d37efdd..a52cf9b 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>
>         req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>                         false, GFP_NOFS);
> -       if (IS_ERR(req)) {
> -               ret = PTR_ERR(req);
> +       if (!req) {
> +               ret = -ENOMEM;
>                 req = orig_req;
>                 goto out;
>         }

Applied, thanks Dan.

Zheng, I have an related concern: where do you put snapc (refcount is
bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
like it's leaked to me.

The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
either -ENOMEM or ceph_osdc_start_request() retval.

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yan, Zheng Jan. 26, 2016, 11:16 a.m. UTC | #2 
> On Jan 26, 2016, at 18:30, Ilya Dryomov <idryomov@gmail.com> wrote:
> 
> On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
>> ceph_osdc_alloc_request() returns NULL on error, it never returns error
>> pointers.
>> 
>> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> 
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index d37efdd..a52cf9b 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>> 
>>        req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>>                        false, GFP_NOFS);
>> -       if (IS_ERR(req)) {
>> -               ret = PTR_ERR(req);
>> +       if (!req) {
>> +               ret = -ENOMEM;
>>                req = orig_req;
>>                goto out;
>>        }
> 
> Applied, thanks Dan.
> 
> Zheng, I have an related concern: where do you put snapc (refcount is
> bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
> like it's leaked to me.
> 
> The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
> either -ENOMEM or ceph_osdc_start_request() retval.

ceph_aio_complete_req treats -EOLDSNAP distinguishingly.  Purpose of this BUG_ON is detect potential infinite loop.

Regards
Yan, Zheng


> 
>                Ilya

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ilya Dryomov Jan. 26, 2016, 11:40 a.m. UTC | #3 
On Tue, Jan 26, 2016 at 12:16 PM, Yan, Zheng <zyan@redhat.com> wrote:
>
>> On Jan 26, 2016, at 18:30, Ilya Dryomov <idryomov@gmail.com> wrote:
>>
>> On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
>> <dan.carpenter@oracle.com> wrote:
>>> ceph_osdc_alloc_request() returns NULL on error, it never returns error
>>> pointers.
>>>
>>> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>> index d37efdd..a52cf9b 100644
>>> --- a/fs/ceph/file.c
>>> +++ b/fs/ceph/file.c
>>> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>>>
>>>        req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>>>                        false, GFP_NOFS);
>>> -       if (IS_ERR(req)) {
>>> -               ret = PTR_ERR(req);
>>> +       if (!req) {
>>> +               ret = -ENOMEM;
>>>                req = orig_req;
>>>                goto out;
>>>        }
>>
>> Applied, thanks Dan.
>>
>> Zheng, I have an related concern: where do you put snapc (refcount is
>> bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
>> like it's leaked to me.
>>
>> The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
>> either -ENOMEM or ceph_osdc_start_request() retval.
>
> ceph_aio_complete_req treats -EOLDSNAP distinguishingly.  Purpose of this BUG_ON is detect potential infinite loop.

Did you miss the part about the snap context?

I get the purpose of -EOLDSNAPC assert in ceph_direct_read_write(),
where you can actually get it from ceph_osdc_wait_request() - it's
a server-side error code.  Asserting it in ceph_aio_retry_work(), in
which only client helpers are called and the only two possible error
codes are -ENOMEM and -EIO doesn't make much sense to me.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yan, Zheng Jan. 26, 2016, 11:54 a.m. UTC | #4 
> On Jan 26, 2016, at 19:40, Ilya Dryomov <idryomov@gmail.com> wrote:
> 
> On Tue, Jan 26, 2016 at 12:16 PM, Yan, Zheng <zyan@redhat.com> wrote:
>> 
>>> On Jan 26, 2016, at 18:30, Ilya Dryomov <idryomov@gmail.com> wrote:
>>> 
>>> On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
>>> <dan.carpenter@oracle.com> wrote:
>>>> ceph_osdc_alloc_request() returns NULL on error, it never returns error
>>>> pointers.
>>>> 
>>>> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> 
>>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>>> index d37efdd..a52cf9b 100644
>>>> --- a/fs/ceph/file.c
>>>> +++ b/fs/ceph/file.c
>>>> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>>>> 
>>>>       req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>>>>                       false, GFP_NOFS);
>>>> -       if (IS_ERR(req)) {
>>>> -               ret = PTR_ERR(req);
>>>> +       if (!req) {
>>>> +               ret = -ENOMEM;
>>>>               req = orig_req;
>>>>               goto out;
>>>>       }
>>> 
>>> Applied, thanks Dan.
>>> 
>>> Zheng, I have an related concern: where do you put snapc (refcount is
>>> bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
>>> like it's leaked to me.
>>> 
>>> The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
>>> either -ENOMEM or ceph_osdc_start_request() retval.
>> 
>> ceph_aio_complete_req treats -EOLDSNAP distinguishingly.  Purpose of this BUG_ON is detect potential infinite loop.
> 
> Did you miss the part about the snap context?
> 
> I get the purpose of -EOLDSNAPC assert in ceph_direct_read_write(),
> where you can actually get it from ceph_osdc_wait_request() - it's
> a server-side error code.  Asserting it in ceph_aio_retry_work(), in
> which only client helpers are called and the only two possible error
> codes are -ENOMEM and -EIO doesn't make much sense to me.
> 

Yeah, removing that BUG_ON is completely OK.

Regards,
Yan, Zheng

> Thanks,
> 
>                Ilya

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ilya Dryomov Jan. 26, 2016, 2:02 p.m. UTC | #5 
On Tue, Jan 26, 2016 at 12:54 PM, Yan, Zheng <zyan@redhat.com> wrote:
>
>> On Jan 26, 2016, at 19:40, Ilya Dryomov <idryomov@gmail.com> wrote:
>>
>> On Tue, Jan 26, 2016 at 12:16 PM, Yan, Zheng <zyan@redhat.com> wrote:
>>>
>>>> On Jan 26, 2016, at 18:30, Ilya Dryomov <idryomov@gmail.com> wrote:
>>>>
>>>> On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
>>>> <dan.carpenter@oracle.com> wrote:
>>>>> ceph_osdc_alloc_request() returns NULL on error, it never returns error
>>>>> pointers.
>>>>>
>>>>> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>
>>>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>>>> index d37efdd..a52cf9b 100644
>>>>> --- a/fs/ceph/file.c
>>>>> +++ b/fs/ceph/file.c
>>>>> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>>>>>
>>>>>       req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>>>>>                       false, GFP_NOFS);
>>>>> -       if (IS_ERR(req)) {
>>>>> -               ret = PTR_ERR(req);
>>>>> +       if (!req) {
>>>>> +               ret = -ENOMEM;
>>>>>               req = orig_req;
>>>>>               goto out;
>>>>>       }
>>>>
>>>> Applied, thanks Dan.
>>>>
>>>> Zheng, I have an related concern: where do you put snapc (refcount is
>>>> bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
>>>> like it's leaked to me.
>>>>
>>>> The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
>>>> either -ENOMEM or ceph_osdc_start_request() retval.
>>>
>>> ceph_aio_complete_req treats -EOLDSNAP distinguishingly.  Purpose of this BUG_ON is detect potential infinite loop.
>>
>> Did you miss the part about the snap context?
>>
>> I get the purpose of -EOLDSNAPC assert in ceph_direct_read_write(),
>> where you can actually get it from ceph_osdc_wait_request() - it's
>> a server-side error code.  Asserting it in ceph_aio_retry_work(), in
>> which only client helpers are called and the only two possible error
>> codes are -ENOMEM and -EIO doesn't make much sense to me.
>>
>
> Yeah, removing that BUG_ON is completely OK.

I still want to know where snapc is put ;)

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ilya Dryomov Jan. 26, 2016, 3:26 p.m. UTC | #6 
On Tue, Jan 26, 2016 at 3:51 PM, Yan, Zheng <zyan@redhat.com> wrote:
>
>> On Jan 26, 2016, at 22:02, Ilya Dryomov <idryomov@gmail.com> wrote:
>>
>> On Tue, Jan 26, 2016 at 12:54 PM, Yan, Zheng <zyan@redhat.com> wrote:
>>>
>>>> On Jan 26, 2016, at 19:40, Ilya Dryomov <idryomov@gmail.com> wrote:
>>>>
>>>> On Tue, Jan 26, 2016 at 12:16 PM, Yan, Zheng <zyan@redhat.com> wrote:
>>>>>
>>>>>> On Jan 26, 2016, at 18:30, Ilya Dryomov <idryomov@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Jan 26, 2016 at 10:24 AM, Dan Carpenter
>>>>>> <dan.carpenter@oracle.com> wrote:
>>>>>>> ceph_osdc_alloc_request() returns NULL on error, it never returns error
>>>>>>> pointers.
>>>>>>>
>>>>>>> Fixes: 5be0389dac66 ('ceph: re-send AIO write request when getting -EOLDSNAP error')
>>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>>
>>>>>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>>>>>> index d37efdd..a52cf9b 100644
>>>>>>> --- a/fs/ceph/file.c
>>>>>>> +++ b/fs/ceph/file.c
>>>>>>> @@ -698,8 +698,8 @@ static void ceph_aio_retry_work(struct work_struct *work)
>>>>>>>
>>>>>>>      req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
>>>>>>>                      false, GFP_NOFS);
>>>>>>> -       if (IS_ERR(req)) {
>>>>>>> -               ret = PTR_ERR(req);
>>>>>>> +       if (!req) {
>>>>>>> +               ret = -ENOMEM;
>>>>>>>              req = orig_req;
>>>>>>>              goto out;
>>>>>>>      }
>>>>>>
>>>>>> Applied, thanks Dan.
>>>>>>
>>>>>> Zheng, I have an related concern: where do you put snapc (refcount is
>>>>>> bumped a few lines above) if ceph_osdc_alloc_request() fails?  It looks
>>>>>> like it's leaked to me.
>>>>>>
>>>>>> The BUG_ON(ret == -EOLDSNAPC) also seems a bit bogus, given that ret is
>>>>>> either -ENOMEM or ceph_osdc_start_request() retval.
>>>>>
>>>>> ceph_aio_complete_req treats -EOLDSNAP distinguishingly.  Purpose of this BUG_ON is detect potential infinite loop.
>>>>
>>>> Did you miss the part about the snap context?
>>>>
>>>> I get the purpose of -EOLDSNAPC assert in ceph_direct_read_write(),
>>>> where you can actually get it from ceph_osdc_wait_request() - it's
>>>> a server-side error code.  Asserting it in ceph_aio_retry_work(), in
>>>> which only client helpers are called and the only two possible error
>>>> codes are -ENOMEM and -EIO doesn't make much sense to me.
>>>>
>>>
>>> Yeah, removing that BUG_ON is completely OK.
>>
>> I still want to know where snapc is put ;)
>>
>
> you are right. I missed that

Great, you can remove that BUG_ON in the same commit then.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index d37efdd..a52cf9b 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -698,8 +698,8 @@  static void ceph_aio_retry_work(struct work_struct *work)
 
 	req = ceph_osdc_alloc_request(orig_req->r_osdc, snapc, 2,
 			false, GFP_NOFS);
-	if (IS_ERR(req)) {
-		ret = PTR_ERR(req);
+	if (!req) {
+		ret = -ENOMEM;
 		req = orig_req;
 		goto out;
 	}