On Mon, 21 Mar 2011 18:54:55 -0400 Trond Myklebust
<Trond.Myklebust@xxxxxxxxxx> wrote:
> On Tue, 2011-03-22 at 09:17 +1100, NeilBrown wrote:
> > On Mon, 21 Mar 2011 17:02:00 -0400 Trond Myklebust
> > <Trond.Myklebust@xxxxxxxxxx> wrote:
> > I must admit that I found the terminology a bit confusing for
> > FLUSH_COND_STABLE.
> > What is means is "if this turns out to be part of a larger request, then clear
> > FLUSH_STABLE" - which sounds a bit more like "FLUSH_COND_UNSTABLE" - i.e. if
> > a condition is met, then make it unstable.
> > But I don't think that change would help at all.
> >
> > How about changing the test in nfs_write_rpcsetup to
> > if (how & (FLUSH_STABLE|FLUSH_CONDSTABLE)) {
> > data->args.stable = NFS_DATA_SYNC;
> > if (!nfs_need_commit(NFS_I(inode)))
> > data->args.stable = NFS_FILE_SYNC;
> > }
> >
> > and then just set either FLUSH_STABLE or FLUSH_COND_STABLE - never both -
> > and when you test FLUSH_COND_STABLE and then some other condition, just
> > clear FLUSH_COND_STABLE.
> >
> > I would find that quite a bit more readable.
>
> The reason why I opted not to go for that approach was because
> nfs_write_rpcsetup() doesn't really know about whether or not there are
> any more RPCs pending (and so having a 'conditional' flag being
> interpreted there didn't appear to make sense), but if you feel that is
> easier on the eyes then I'm happy to change my opinion.
>
I do see your point - by the time we get to nfs_write_rpcsetup it isn't
really conditional any more - it is now mandatory.
Maybe one cannot get the names perfect. However I find that having
interdependencies between different flag bits can easily become confusing.
So I have a slight preference for my version, but I concede that it isn't a
clear winner.
Thanks,
NeilBrown
--
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
