Re: [PATCH 3/5] tmpfs: handle MPOL_LOCAL mount option properly

[Hugh Dickins]

On Wed, 17 Mar 2010, Lee Schermerhorn wrote:
> On Thu, 2010-03-18 at 08:52 +0900, KOSAKI Motohiro wrote:
> > > On Tue, 16 Mar 2010, KOSAKI Motohiro wrote:
> > > 
> > > > commit 71fe804b6d5 (mempolicy: use struct mempolicy pointer in
> > > > shmem_sb_info) added mpol=local mount option. but its feature is
> > > > broken since it was born. because such code always return 1 (i.e.
> > > > mount failure).
> > > > 
> > > > This patch fixes it.
> > > > 
> > > > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> > > > Cc: Ravikiran Thirumalai <kiran@xxxxxxxxxxxx>
> > > 
> > > Thank you both for finding and fixing these mpol embarrassments.
> > > 
> > > But if this "mpol=local" feature was never documented (not even in the
> > > commit log),

Sorry, I was absolutely wrong to say that: I seem to have been looking at
the wrong commit, but 3f226aa1 does document mpol=local, and does explain
why mpol=default differed; and KOSAKI-san's 5/5 corrects the mpol=default
description in tmpfs.txt, as well as adding mpol=local description.


> > > has been broken since birth 20 months ago, and nobody has
> > > noticed: wouldn't it be better to save a little bloat and just rip it out?
> > 
> > I have no objection if lee agreed, lee?
> > Of cource, if we agree it, we can make the new patch soon :)
> > 
> 
> Well, given the other issues with mpol_parse_str(), I suspect the entire
> tmpfs mpol option is not used all that often in the mainline kernel.  I
> recall that this feature was introduced by SGI for some of their
> customers who may depend on it.  There have been cases where I could
> have used it were it supported for the SYSV shm and MAP_ANON_MAP_SHARED
> internal tmpfs mount.  Further, note that the addition of "mpol=local"
> occurred between when the major "enterprise distributions" selected a
> new mainline kernel.  Production users of those distros, who are the
> likely users of this feature, tend not to live on the bleeding edge.
> So, maybe we shouldn't read too much into it not being discovered until
> now.

True.

> 
> That being said, I suppose I wouldn't be all that opposed to deprecating
> the entire tmpfs mpol option, and see who yells.   If the mpol mount
> options stays, I'd like to see the 'local' option stay.  It's a
> legitimate behavior that one can specify via the system calls and see
> via numa_maps, so I think the mpol mount option, if it exists, should
> support a way to specify it.  
> 
> As for bloat, the additional code on the mount option side to support it
> is:
> 
>         case MPOL_LOCAL:
>                 /*
>                  * Don't allow a nodelist;  mpol_new() checks flags
>                  */
>                 if (nodelist)
>                         goto out;
>                 mode = MPOL_PREFERRED;
>                 break;

Shocking :)

Yes, I withdraw my objection.  I'd been puzzled by why you would have
added an option which nobody was interested in, including yourself;
but as I see it now, you'd noticed a bug in mpol=default, and felt
that the right way to provide the missing functionality was to add
mpol=local: fair enough, let's keep it.

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/