On 2011-06-01 21:07, Trond Myklebust wrote: > On Wed, 2011-06-01 at 17:51 +0300, Benny Halevy wrote: >> I think the following should work: >> >> Benny >> >> git diff --stat -p -M >> fs/nfs/nfs4filelayout.c | 10 ++++++++++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> >> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c >> index 4269088..9f1d445 100644 >> --- a/fs/nfs/nfs4filelayout.c >> +++ b/fs/nfs/nfs4filelayout.c >> @@ -661,6 +661,16 @@ filelayout_pg_test(struct nfs_pageio_descriptor >> *pgio, struct nfs_page *prev, >> u64 p_stripe, r_stripe; >> u32 stripe_unit; >> >> + /* >> + * FIXME: ideally we should be able to coalesce all requests >> + * that are not block boundary aligned, but currently this >> + * is problematic for the case of bsize < PAGE_CACHE_SIZE, >> + * since nfs_flush_multi and nfs_pagein_multi assume you >> + * can have only one struct nfs_page. >> + */ >> + if (desc->pg_bsize < PAGE_SIZE) >> + return 0; >> + >> if (!pnfs_generic_pg_test(pgio, prev, req)) >> return 0; > > So, there are several things that bother me about pnfs_generic_pg_test() > too now that I'm looking more closely at it: > > 1. If the intention is to coalesce 'prev' and 'req', shouldn't we > be asking for a layout with req_offset(prev) instead of > req_offset(req)? > 2. If we're only requesting a layout of length pg_count, don't we > still need to test the layout length that the server actually > returned before we can allow the coalescing? > 3. if (!pgio->lseg), shouldn't we be returning an error of some > sort? Right now we're returning 'true', and allowing the > coalesce to occur. > 4. Furthermore, shouldn't that test guarding the > pnfs_update_layout() call rather be an 'if (pgio->pg_lseg == > NULL)' instead of looking at the values of pg_count and > prev->wb_bytes? > or rather we get the layout for the first page in nfs_pageio_do_add_request when desc->pg_count == 0? Then, this lseg would be useful for nfs_flush_multi if we failed to coalesce, or we failed to get a layout altogether we go the nfs path and can reset pg_test to nfs_generic_pg_test. Otherwise I agree with your assertions above. Benny -- 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
