On 2011-10-04 06:34, Boaz Harrosh wrote:
> The EOF calculation was done on .read_pagelist(), cached
> in objlayout_io_state->eof, and set in objlayout_read_done()
> into nfs_read_data->res.eof.
>
> So set it directly into nfs_read_data->res.eof and avoid
> the extra member.
>
> This is a slight behaviour change because before eof was
> *not* set on an error update at objlayout_read_done(). But
> is that a problem? Is Generic layer so sensitive that it
> will miss the error IO if eof was set? From my testing
> I did not see such a problem.
>
> Benny please review.
>
> Which brings me to a more abstract problem. Why does the
> LAYOUT driver needs to do this eof calculation? .i.e we
> are inspecting generic i_size_read() and if spanned by
> offset + count which is received from generic layer we set
> eof. It looks like all this can/should be done in generic
> layer and not at LD. Where does NFS and files-LD do it?
> It looks like it can be promoted.
In the files layout case, nfs_read_done sets res.eof.
But I agree this code could be moved to the generic layout
at least to serve non-rpc LDs.
And BTW, current the object layout handling of the eof flag
is stricter than the blocks layout and it requires an extra
call with offset >= i_size to set the eof flag, while for
nfs and blocks eof is set when offset + count >= i_size
>
> Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
Reviewed-by: Benny Halevy <bhalevy@xxxxxxxxxx>
> ---
> fs/nfs/objlayout/objlayout.c | 16 +++++++---------
> fs/nfs/objlayout/objlayout.h | 1 -
> 2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/objlayout/objlayout.c b/fs/nfs/objlayout/objlayout.c
> index 1d06f8e..1300736 100644
> --- a/fs/nfs/objlayout/objlayout.c
> +++ b/fs/nfs/objlayout/objlayout.c
> @@ -287,17 +287,14 @@ static void _rpc_read_complete(struct work_struct *work)
> void
> objlayout_read_done(struct objlayout_io_state *state, ssize_t status, bool sync)
> {
> - int eof = state->eof;
> - struct nfs_read_data *rdata;
> + struct nfs_read_data *rdata = state->rpcdata;
>
> state->status = status;
> - dprintk("%s: Begin status=%zd eof=%d\n", __func__, status, eof);
> - rdata = state->rpcdata;
> + dprintk("%s: Begin status=%zd eof=%d\n", __func__,
> + status, rdata->res.eof);
> rdata->task.tk_status = status;
> - if (status >= 0) {
> + if (status >= 0)
> rdata->res.count = status;
> - rdata->res.eof = eof;
> - }
> objlayout_iodone(state);
> /* must not use state after this point */
>
> @@ -330,11 +327,14 @@ objlayout_read_pagelist(struct nfs_read_data *rdata)
> status = 0;
> rdata->res.count = 0;
> rdata->res.eof = 1;
> + /*FIXME: do we need to call pnfs_ld_read_done() */
Yes, it looks like we do, otherwise we might leak a refcount on the lseg.
We also need to set rdata->task.tk_status = 0, to mimic what objlayout_read_done
would have done in the sync case.
Benny
> goto out;
> }
> count = eof - offset;
> }
>
> + rdata->res.eof = (offset + count) >= eof;
> +
> state = objlayout_alloc_io_state(NFS_I(rdata->inode)->layout,
> rdata->args.pages, rdata->args.pgbase,
> offset, count,
> @@ -345,8 +345,6 @@ objlayout_read_pagelist(struct nfs_read_data *rdata)
> goto out;
> }
>
> - state->eof = state->offset + state->count >= eof;
> -
> status = objio_read_pagelist(state);
> out:
> dprintk("%s: Return status %Zd\n", __func__, status);
> diff --git a/fs/nfs/objlayout/objlayout.h b/fs/nfs/objlayout/objlayout.h
> index a8244c8..ffb884c 100644
> --- a/fs/nfs/objlayout/objlayout.h
> +++ b/fs/nfs/objlayout/objlayout.h
> @@ -86,7 +86,6 @@ struct objlayout_io_state {
>
> void *rpcdata;
> int status; /* res */
> - int eof; /* res */
> int committed; /* res */
>
> /* Error reporting (layout_return) */
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
