On Wed, 1 Apr 2009 14:33:23 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
Sorry I'm late. I know Steve has already committed this, but here are
some comments. Maybe we can fix them in a later patch...
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 9fbf4df..9a8368f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -350,7 +350,7 @@ struct cifsFileInfo {
> bool invalidHandle:1; /* file closed via session abend */
> bool messageMode:1; /* for pipes: message vs byte mode */
> atomic_t wrtPending; /* handle in use - defer close */
> - struct semaphore fh_sem; /* prevents reopen race after dead ses*/
> + struct mutex fh_mutex; /* prevents reopen race after dead ses*/
> struct cifs_search_info srch_inf;
> };
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index f9b6f68..f473c73 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -129,12 +129,64 @@ cifs_bp_rename_retry:
> return full_path;
> }
>
> +static void
> +cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
> + struct cifsTconInfo *tcon, bool write_only)
> +{
> + int oplock = 0;
> + struct cifsFileInfo *pCifsFile;
> + struct cifsInodeInfo *pCifsInode;
> +
> + pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
> +
> + if (pCifsFile == NULL)
> + return;
> +
> + if (oplockEnabled)
> + oplock = REQ_OPLOCK;
> +
> + pCifsFile->netfid = fileHandle;
> + pCifsFile->pid = current->tgid;
> + pCifsFile->pInode = newinode;
> + pCifsFile->invalidHandle = false;
> + pCifsFile->closePend = false;
^^^^^^^
None of the other '=' are lined up. Why here?
> + mutex_init(&pCifsFile->fh_mutex);
> + mutex_init(&pCifsFile->lock_mutex);
> + INIT_LIST_HEAD(&pCifsFile->llist);
> + atomic_set(&pCifsFile->wrtPending, 0);
> +
> + /* set the following in open now
> + pCifsFile->pfile = file; */
> + write_lock(&GlobalSMBSeslock);
> + list_add(&pCifsFile->tlist, &tcon->openFileList);
> + pCifsInode = CIFS_I(newinode);
> + if (pCifsInode) {
> + /* if readable file instance put first in list*/
> + if (write_only) {
> + list_add_tail(&pCifsFile->flist,
> + &pCifsInode->openFileList);
> + } else {
> + list_add(&pCifsFile->flist,
> + &pCifsInode->openFileList);
> + }
^^^^^^^^
Are these brackets needed?
> + if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
> + pCifsInode->clientCanCacheAll = true;
> + pCifsInode->clientCanCacheRead = true;
> + cFYI(1, ("Exclusive Oplock inode %p",
> + newinode));
> + } else if ((oplock & 0xF) == OPLOCK_READ)
> + pCifsInode->clientCanCacheRead = true;
> + }
Ditto ^^^^^^^^^
> + write_unlock(&GlobalSMBSeslock);
> +}
> +
> int cifs_posix_open(char *full_path, struct inode **pinode,
> struct super_block *sb, int mode, int oflags,
> int *poplock, __u16 *pnetfid, int xid)
> {
> int rc;
> __u32 oplock;
> + bool write_only = false;
> FILE_UNIX_BASIC_INFO *presp_data;
> __u32 posix_flags = 0;
> struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> @@ -172,6 +224,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> if (oflags & O_DIRECT)
> posix_flags |= SMB_O_DIRECT;
>
> + if (!(oflags & FMODE_READ))
> + write_only = true;
>
> rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
> pnetfid, presp_data, &oplock, full_path,
> @@ -198,6 +252,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>
> posix_fill_in_inode(*pinode, presp_data, 1);
>
> + cifs_fill_fileinfo(*pinode, *pnetfid, cifs_sb->tcon, write_only);
> +
> posix_open_ret:
> kfree(presp_data);
> return rc;
> @@ -239,7 +295,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> char *full_path = NULL;
> FILE_ALL_INFO *buf = NULL;
> struct inode *newinode = NULL;
> - struct cifsInodeInfo *pCifsInode;
> int disposition = FILE_OVERWRITE_IF;
> bool write_only = false;
>
> @@ -410,44 +465,8 @@ cifs_create_set_dentry:
> /* mknod case - do not leave file open */
> CIFSSMBClose(xid, tcon, fileHandle);
> } else if (newinode) {
> - struct cifsFileInfo *pCifsFile =
> - kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
> -
> - if (pCifsFile == NULL)
> - goto cifs_create_out;
> - pCifsFile->netfid = fileHandle;
> - pCifsFile->pid = current->tgid;
> - pCifsFile->pInode = newinode;
> - pCifsFile->invalidHandle = false;
> - pCifsFile->closePend = false;
> - init_MUTEX(&pCifsFile->fh_sem);
> - mutex_init(&pCifsFile->lock_mutex);
> - INIT_LIST_HEAD(&pCifsFile->llist);
> - atomic_set(&pCifsFile->wrtPending, 0);
> -
> - /* set the following in open now
> - pCifsFile->pfile = file; */
> - write_lock(&GlobalSMBSeslock);
> - list_add(&pCifsFile->tlist, &tcon->openFileList);
> - pCifsInode = CIFS_I(newinode);
> - if (pCifsInode) {
> - /* if readable file instance put first in list*/
> - if (write_only) {
> - list_add_tail(&pCifsFile->flist,
> - &pCifsInode->openFileList);
> - } else {
> - list_add(&pCifsFile->flist,
> - &pCifsInode->openFileList);
> - }
> - if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
> - pCifsInode->clientCanCacheAll = true;
> - pCifsInode->clientCanCacheRead = true;
> - cFYI(1, ("Exclusive Oplock inode %p",
> - newinode));
> - } else if ((oplock & 0xF) == OPLOCK_READ)
> - pCifsInode->clientCanCacheRead = true;
> - }
> - write_unlock(&GlobalSMBSeslock);
> + cifs_fill_fileinfo(newinode, fileHandle,
> + cifs_sb->tcon, write_only);
> }
> cifs_create_out:
> kfree(buf);
> @@ -580,17 +599,21 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
> return rc;
> }
>
> -
> struct dentry *
> cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> struct nameidata *nd)
> {
> int xid;
> int rc = 0; /* to get around spurious gcc warning, set to zero here */
> + int oplock = 0;
> + int mode;
> + __u16 fileHandle = 0;
> + bool posix_open = false;
> struct cifs_sb_info *cifs_sb;
> struct cifsTconInfo *pTcon;
> struct inode *newInode = NULL;
> char *full_path = NULL;
> + struct file *filp;
>
> xid = GetXid();
>
> @@ -632,12 +655,27 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> }
> cFYI(1, ("Full path: %s inode = 0x%p", full_path, direntry->d_inode));
>
> - if (pTcon->unix_ext)
> - rc = cifs_get_inode_info_unix(&newInode, full_path,
> - parent_dir_inode->i_sb, xid);
> - else
> + if (pTcon->unix_ext) {
> + if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
> + (nd->flags & LOOKUP_OPEN)) {
> + if (!((nd->intent.open.flags & O_CREAT) &&
> + (nd->intent.open.flags & O_EXCL))) {
> + mode = nd->intent.open.create_mode &
> + ~current->fs->umask;
> + rc = cifs_posix_open(full_path, &newInode,
> + parent_dir_inode->i_sb, mode,
> + nd->intent.open.flags, &oplock,
> + &fileHandle, xid);
> + if ((rc != -EINVAL) && (rc != -EOPNOTSUPP))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This looks wrong to me. It's possible that you'd get a different
return code here on an unsuccessful open call? Why would we want to
instantiate the filp on any error from cifs_posix_open? For instance,
that function can return -ENOMEM. I don't think we want to instantiate
the filp in that case.
Maybe that should just be: "if (!rc)" ?
> + posix_open = true;
> + }
> + }
> + if (!posix_open)
> + rc = cifs_get_inode_info_unix(&newInode, full_path,
> + parent_dir_inode->i_sb, xid);
> + } else
> rc = cifs_get_inode_info(&newInode, full_path, NULL,
> - parent_dir_inode->i_sb, xid, NULL);
> + parent_dir_inode->i_sb, xid, NULL);
>
> if ((rc == 0) && (newInode != NULL)) {
> if (pTcon->nocase)
> @@ -645,7 +683,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> else
> direntry->d_op = &cifs_dentry_ops;
> d_add(direntry, newInode);
> -
> + if (posix_open)
> + filp = lookup_instantiate_filp(nd, direntry, NULL);
> /* since paths are not looked up by component - the parent
> directories are presumed to be good here */
> renew_parental_timestamps(direntry);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 81747ac..c3f51de 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -46,7 +46,7 @@ static inline struct cifsFileInfo *cifs_init_private(
> memset(private_data, 0, sizeof(struct cifsFileInfo));
> private_data->netfid = netfid;
> private_data->pid = current->tgid;
> - init_MUTEX(&private_data->fh_sem);
> + mutex_init(&private_data->fh_mutex);
> mutex_init(&private_data->lock_mutex);
> INIT_LIST_HEAD(&private_data->llist);
> private_data->pfile = file; /* needed for writepage */
> @@ -284,35 +284,34 @@ int cifs_open(struct inode *inode, struct file *file)
> cifs_sb = CIFS_SB(inode->i_sb);
> tcon = cifs_sb->tcon;
>
> - if (file->f_flags & O_CREAT) {
> - /* search inode for this file and fill in file->private_data */
> - pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> - read_lock(&GlobalSMBSeslock);
> - list_for_each(tmp, &pCifsInode->openFileList) {
> - pCifsFile = list_entry(tmp, struct cifsFileInfo,
> - flist);
> - if ((pCifsFile->pfile == NULL) &&
> - (pCifsFile->pid == current->tgid)) {
> - /* mode set in cifs_create */
> -
> - /* needed for writepage */
> - pCifsFile->pfile = file;
> -
> - file->private_data = pCifsFile;
> - break;
> - }
> - }
> - read_unlock(&GlobalSMBSeslock);
> - if (file->private_data != NULL) {
> - rc = 0;
> - FreeXid(xid);
> - return rc;
> - } else {
> - if (file->f_flags & O_EXCL)
> - cERROR(1, ("could not find file instance for "
> - "new file %p", file));
> + /* search inode for this file and fill in file->private_data */
> + pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
> + read_lock(&GlobalSMBSeslock);
> + list_for_each(tmp, &pCifsInode->openFileList) {
> + pCifsFile = list_entry(tmp, struct cifsFileInfo,
> + flist);
> + if ((pCifsFile->pfile == NULL) &&
> + (pCifsFile->pid == current->tgid)) {
> + /* mode set in cifs_create */
> +
> + /* needed for writepage */
> + pCifsFile->pfile = file;
> +
> + file->private_data = pCifsFile;
> + break;
> }
> }
> + read_unlock(&GlobalSMBSeslock);
> +
> + if (file->private_data != NULL) {
> + rc = 0;
> + FreeXid(xid);
> + return rc;
> + } else {
> + if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
^^^^^ how about "else if" here and reduce the indentation below?
> + cERROR(1, ("could not find file instance for "
> + "new file %p", file));
> + }
>
> full_path = build_path_from_dentry(file->f_path.dentry);
> if (full_path == NULL) {
> @@ -500,9 +499,9 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
> return -EBADF;
>
> xid = GetXid();
> - down(&pCifsFile->fh_sem);
> + mutex_unlock(&pCifsFile->fh_mutex);
> if (!pCifsFile->invalidHandle) {
> - up(&pCifsFile->fh_sem);
> + mutex_lock(&pCifsFile->fh_mutex);
> FreeXid(xid);
> return 0;
> }
> @@ -533,7 +532,7 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
> if (full_path == NULL) {
> rc = -ENOMEM;
> reopen_error_exit:
> - up(&pCifsFile->fh_sem);
> + mutex_lock(&pCifsFile->fh_mutex);
> FreeXid(xid);
> return rc;
> }
> @@ -575,14 +574,14 @@ reopen_error_exit:
> cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
> CIFS_MOUNT_MAP_SPECIAL_CHR);
> if (rc) {
> - up(&pCifsFile->fh_sem);
> + mutex_lock(&pCifsFile->fh_mutex);
> cFYI(1, ("cifs_open returned 0x%x", rc));
> cFYI(1, ("oplock: %d", oplock));
> } else {
> reopen_success:
> pCifsFile->netfid = netfid;
> pCifsFile->invalidHandle = false;
> - up(&pCifsFile->fh_sem);
> + mutex_lock(&pCifsFile->fh_mutex);
> pCifsInode = CIFS_I(inode);
> if (pCifsInode) {
> if (can_flush) {
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
