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

[Lee Schermerhorn]

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), 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.

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;

Lee



--
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/