On Wed, 30 Nov 2011 08:56:57 -0600
Malahal Naineni <malahal@xxxxxxxxxx> wrote:
> Jeff Layton [jlayton@xxxxxxxxxx] wrote:
> > When given a range to write with unstable writes, the current code
> > always does a COMMIT of the entire file afterward. This is potentially
> > expensive on some servers and unnecessary. Instead, just do a COMMIT for
> > the offset and count that was written.
> >
> > Khoa, who reported this bug, stated that this made a big difference in
> > performance in their environment, which I believe involves GPFS on the
> > server. He didn't pass along any hard numbers so I can't quantify the
> > gain, but it stands to reason that clustered filesystems might suffer
> > more contention issues when issuing a commit over the whole file.
> >
> > Reported-by: Khoa Huynh <khoa@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/nfs/direct.c | 6 ++++--
> > 1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> > index 1940f1a..33f2be7 100644
> > --- a/fs/nfs/direct.c
> > +++ b/fs/nfs/direct.c
> > @@ -77,6 +77,7 @@ struct nfs_direct_req {
> > /* completion state */
> > atomic_t io_count; /* i/os we're waiting for */
> > spinlock_t lock; /* protect completion state */
> > + loff_t pos; /* offset into file */
>
> Why not call it "offset"?
Why should we call it "offset"? The rest of the code refers to this
value as "pos". Essentially this patch replaces a field that was
removed from this struct several years ago. At the time, that field was
also called "pos".
> Should we set this to zero in
> nfs_direct_req_alloc (just like count is set to zero there)?
>
Yes, that certainly wouldn't hurt. I'll plan to add that into the next
respin which I'll plan to send out once I've collected some more
feedback.
Thanks,
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
