On Thu, Dec 30, 2010 at 8:29 PM, Steve French <smfrench@xxxxxxxxx> wrote:
> On Thu, Dec 30, 2010 at 9:20 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>
>> On Thu, 30 Dec 2010 18:14:34 +0300
>> Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>>
>> > 2010/12/30 Jeff Layton <jlayton@xxxxxxxxxx>:
>> > > ...and remove length qualifiers from bools.
>> > >
>> > > Before:
>> > >
>> > > /* size: 1176, cachelines: 19, members: 13 */
>> > > /* sum members: 1165, holes: 2, sum holes: 11 */
>> > > /* bit holes: 1, sum bit holes: 4 bits */
>> > > /* last cacheline: 24 bytes */
>> > >
>> > > After:
>> > >
>> > > /* size: 1168, cachelines: 19, members: 13 */
>> > > /* last cacheline: 16 bytes */
>> > >
>> > > ...savings of 8 bytes per inode.
>> > >
>> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> > > ---
>> > > fs/cifs/cifsglob.h | 10 +++++-----
>> > > 1 files changed, 5 insertions(+), 5 deletions(-)
>> > >
>> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> > > index d6433a4..701060d 100644
>> > > --- a/fs/cifs/cifsglob.h
>> > > +++ b/fs/cifs/cifsglob.h
>> > > @@ -439,11 +439,11 @@ struct cifsInodeInfo {
>> > > /* BB add in lists for dirty pages i.e. write caching info for oplock */
>> > > struct list_head openFileList;
>> > > __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
>> > > - unsigned long time; /* jiffies of last update/check of inode */
>> > > - bool clientCanCacheRead:1; /* read oplock */
>> > > - bool clientCanCacheAll:1; /* read and writebehind oplock */
>> > > - bool delete_pending:1; /* DELETE_ON_CLOSE is set */
>> > > - bool invalid_mapping:1; /* pagecache is invalid */
>> > > + bool clientCanCacheRead; /* read oplock */
>> > > + bool clientCanCacheAll; /* read and writebehind oplock */
>> > > + bool delete_pending; /* DELETE_ON_CLOSE is set */
>> > > + bool invalid_mapping; /* pagecache is invalid */
>> > > + unsigned long time; /* jiffies of last update of inode */
>> > > u64 server_eof; /* current file size on server */
>> > > u64 uniqueid; /* server inode number */
>> > > u64 createtime; /* creation time on server */
>> > > --
>> > > 1.7.3.4
>> > >
>> > > --
>> > > 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
>> > >
>> >
>> > Looks good. May be we should rename clientCanCache* to client_can_cache_*?
>> >
>> > Anyway - Reviewed-by: Pavel Shilovsky <piastryyy@xxxxxxxxx>
>> >
>>
>> No objection to renaming fields. I actually wouldn't mind just keeping
>> the oplock flag as sent by the server either rather than translating it
>> into bools. Either way though, that should be a separate patch...
>
> I don't mind booleans - on some archs the space saved can be
> significant. but in any case renaming fields from bool seems much
> lower priority than some of the other patches you all have submitted
> recently that add function or improve stability.
I had been thinking about requesting batch oplock for a while to
improve caching of files which are only briefly closed and to allow
multiple opens from the same client to be cached but decided to punt
on this for cifs and just do it for smb2 (don't mind cifs patches for
it - but it is not trivial). Fortunately in smb2 other oplock
improvements help even more.
--
Thanks,
Steve
--
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
