Re: [PATCH 05/16] cifs: eliminate oflags option from cifs_new_fileinfo

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 11 Oct 2010 14:14:36 -0500
Steve French <smfrench@xxxxxxxxx> wrote:

> On Mon, Oct 11, 2010 at 2:07 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > Eliminate the poor, misunderstood "oflags" option from cifs_new_fileinfo.
> > The callers mostly pass in the filp->f_flags here.
> >
> > That's not correct however since we're checking that value for
> > the presence of FMODE_READ. Luckily that only affects how the f_list is
> > ordered. What it really wants here is the file->f_mode, but this check
> > really makes no sense whatsoever. FMODE_READ will be set for O_RDWR or
> > O_RDONLY opens. So this in effect just moves those to the front of the
> > list and leaves O_WRONLY at the end.
> >
> > That might make some sense if more things paid attention to the list
> > order, but the only thing that does is find_readable_file, which uses
> > it to stop scanning the list when it hits a write-only one.
> 
> Removing a small optimization for small simplification of code seems
> less useful type of patch.   Not sure how often the acl calls get hit
> - but it is remotely possible that this could affect performance.
> 

Emphasis on _remotely_ :)

They don't get hit at all unless you're mounting with -o cifsacl. If
you're doing that, performance is already going to suck and this won't
make the least bit of difference.

It's really hard to see how this would have any measurable performance
impact. I think we'll all be better off with clear, defensive code for
this sort of stuff rather than questionable optimizations.

-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux