On Thu, 07 Oct 2010 14:18:50 +0530
Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
> On 10/07/2010 01:24 AM, Jeff Layton wrote:
> > The count for cifsFileInfo is currently an atomic, but that just adds
> > complexity for little value. We generally need to hold cifs_file_list_lock
> > to traverse the lists anyway so we might as well make this counter
> > non-atomic and simply use the cifs_file_list_lock to protect it.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/cifs/cifsglob.h | 9 ++++++---
> > fs/cifs/file.c | 8 +++++---
> > 2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 531a768..f3c4e00 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -393,16 +393,19 @@ struct cifsFileInfo {
> > struct list_head llist; /* list of byte range locks we have. */
> > bool invalidHandle:1; /* file closed via session abend */
> > bool oplock_break_cancelled:1;
> > - atomic_t count; /* reference count */
> > + int count; /* refcount -- protected by cifs_file_list_lock */
> > struct mutex fh_mutex; /* prevents reopen race after dead ses*/
> > struct cifs_search_info srch_inf;
> > struct work_struct oplock_break; /* work for oplock breaks */
> > };
> >
> > -/* Take a reference on the file private data */
> > +/*
> > + * Take a reference on the file private data. Must be called with
> > + * cifs_file_list_lock held for read or write.
> > + */
> > static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
> > {
> > - atomic_inc(&cifs_file->count);
> > + ++cifs_file->count;
>
> Since we now use cifs_file_list_lock to protect cifs_file->count too,
> shouldn't all the callers of cifsFileInfo_get() need to acquire a write
> lock instead of read lock?
>
I puzzled over that too, but it's not really necessary. The lock is
really there to make sure that the list traversals are safe from
removals. We do need the write lock when inserting or removing the
cifsFileInfo into the lists, but for incrementing a non-zero refcount a
read lock is sufficient. That will prevent cifsFileInfo_put from
racing in, decrementing the refcount and modifying the list since it
needs a write lock.
TBH, I think it would be far simpler (and maybe even more efficient) to
use a regular spinlock here, but I figured this would be easier to get
past review.
Thoughts?
--
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
