[Ocfs2-tools-devel] [PATCH 4/6] O2info: Add running codes for '--fs-features'.

[tristan]

Joel Becker wrote:
> On Fri, Apr 23, 2010 at 03:34:53PM -0700, Sunil Mushran wrote:
>   
>>> +	rc = ioctl(fd, OCFS2_IOC_INFO, &info);
>>> +	if (rc) {
>>> +		rc = errno;
>>> +		o2i_error(op, "ioctl failed: %s\n", strerror(rc));
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (oif.if_req.ir_flags & OCFS2_INFO_FL_FILLED) {
>>> +		ofs->compat = oif.if_compat_features;
>>> +		ofs->incompat = oif.if_incompat_features;
>>> +		ofs->rocompat = oif.if_ro_compat_features;
>>> +	}
>>> +
>>> +out:
>>> +	return rc;
>>> +}
>>>   
>>>       
>> What happens if rc == 0 and flags is not FILLED. Best if
>> we ensure that cannot happen. As in, kernel should return
>> an error if it is not filled.
>>     
>
> 	He's correct here.  Remember, we allow !FILLED for forwards
> compatibility.  The kernel skips requests it doesn't understand rather
> than returning an error.  So if you pass N requests, you have to check
> each one to see if FILLED was set to determine whether the kernel
> understood it.  The 0 just means there was no actual error.
> 	But this brings up a good point.  As currently defined, we
> return our first error.  Imagine that we fill the first six requests out
> of an info_count of eight.  Then the seventh request has a bad pointer.
> Currently we return -EFAULT, and the caller misses the six finished
> requests.
> 	I can think of three solutions.  First, we can just leave it
> as-is - if there is any error, you get an error and assume nothing is
> filled in.
> 	Next, we could go with read(2) semantics.  Rather than returning
> 0 on success, we return the number of filled requests.  We only return
> an error if we had the error before we filled any request.  Say you have
> two requests.  The first request is successfully filled.  The second has
> a bad pointer.  You return 1.  The caller tries again passing only the
> second request.  Now they get -EFAULT.  Unknown requests are skipped.
> So if you pass in eight requests, and the call returns 4, you only
> need to walk the requests until you've found four that were FILLED.
> 	The biggest downside with this method is the same as for
> read(2): if you get a short count, you have to call again to find out
> why.
>
> --8<-------------------------------------------------------------------
> retry:
> 	rc = ioctl(fd, INFO, &info);
> 	if (rc < 0)
> 		goto error;
> 	assert(rc <= info.info_coutn);
> 	for (i = 0, seen = 0; i < rc; i++) {
> 		if (reqs[i].ir_flags & FILLED) {
> 			/* do something */
> 			seen++;
> 		}
> 	}
> 	/* Did we get to the end */
> 	if ((rc < info.info_count) && (i < info.info_count)) {
> 		info.info_requests = info.info_requests + i;
> 		info.info_count -= info.info_count;
> 		goto retry;
> 	}
> -->8-------------------------------------------------------------------
>
> 	That's pretty clunky for every user to do.  Of course, we can
> wrap in the library somewhat, so that the retry is handled by the
> library car.
> 	Finally, we could do it ASMLib style.  Add req.ir_error and
> OCFS2_INFO_FL_ERROR.  Here, we only return an error from the ioctl(2)
> when the failure was in the ioctl(2) call itself.  So the ioctl(2)
> returns 0 if it exists and the info structure is valid.  Any error on a
> particular request is set in ir_error; this causes FL_ERROR to be set.
> 	The caller now walks the requests and can determined that
> FL_ERROR is an error, FL_FILLED is completed, and neither means the
> request wasn't supported.
>   

Hi Joel,

I loved your new solutions;), how about we incorporate solution #2 and #3?

We do it mostly like solution #3, but also returning successful FILLED 
count by make info.info_count as 'in/out'.

That way, we still return the real error code from rc of ioctl(2) no 
matter what the error is(in ioctl() itself or in meaty filling 
operation). also with corresponding FL_ERROR being set correctly in each 
request.

Then we'll be able to judge the situation from userspace by 
distinguishing following 3 cases:

1. rc == 0, info_count == full_count: all requests get successfully 
handled, no need to check FL_ERROR.

2. rc == 0, info_count < full_count: means some unknown request didn't 
get filled, no need to check FL_ERROR.

3. rc != 0: real error in kernel happens, if all FL_ERROR were ok in 
each request, we're sure the error happened in ioctl() before or after 
real request filling, otherwise, the error should be in the operation of 
each request handling.


with above scheme, I've had another concern:

Should we immediately return the ioctl(2) when hitting error in #N 
request(N< inf_count), or we continuously to handle the rest requests by 
just simply set the FL_ERROR of #N request?

Tristan.

> Joel
>
>