* [linux-cifs-client][patch] utilize lookup intents to open in lookup
@ 2009-03-31 16:57 Shirish Pargaonkar
2009-04-01 19:33 ` Shirish Pargaonkar
0 siblings, 1 reply; 5+ messages in thread
From: Shirish Pargaonkar @ 2009-03-31 16:57 UTC (permalink / raw)
To: linux-cifs-client, linux-fsdevel; +Cc: Steve French, Trond Myklebust, viro
[-- Attachment #1: Type: text/plain, Size: 255 bytes --]
Hi,
Patch to expolint lookup intents in cifs lookup during open, thus
reducing network traffick and speedier opens
Have also converted a semphore to mutex as per Jeff Layton's comments.
Trong, Al, your feedback is really appreciated.
Regards,
Shirish
[-- Attachment #2: li.4.patch --]
[-- Type: application/octet-stream, Size: 10439 bytes --]
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..3ccb900 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -129,12 +129,67 @@ 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;
+
+ cERROR(1, ("cifs_fill_fileinfo enter\n"));
+
+ 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;
+ 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);
+ }
+ 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);
+ cERROR(1, ("cifs_fill_fileinfo exit\n"));
+}
+
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 +227,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 +255,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 +298,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 +468,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 +602,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 +658,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))
+ 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 +686,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))
+ 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) {
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [linux-cifs-client][patch] utilize lookup intents to open in lookup
2009-03-31 16:57 [linux-cifs-client][patch] utilize lookup intents to open in lookup Shirish Pargaonkar
@ 2009-04-01 19:33 ` Shirish Pargaonkar
2009-04-09 12:55 ` Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Shirish Pargaonkar @ 2009-04-01 19:33 UTC (permalink / raw)
To: linux-cifs-client, linux-fsdevel; +Cc: Steve French, Trond Myklebust, viro
[-- Attachment #1: Type: text/plain, Size: 580 bytes --]
On Tue, Mar 31, 2009 at 11:57 AM, Shirish Pargaonkar
<shirishpargaonkar@gmail.com> wrote:
> Hi,
>
> Patch to expolint lookup intents in cifs lookup during open, thus
> reducing network traffick and speedier opens
> Have also converted a semphore to mutex as per Jeff Layton's comments.
>
> Trong, Al, your feedback is really appreciated.
>
> Regards,
>
> Shirish
>
Forgot to take out two debug lines. Did test this patch while
creating regular files, directories, symbolic links,
exclusive create against older and newer version of samba and so far it works.
Regards,
Shirish
[-- Attachment #2: li.5.patch --]
[-- Type: application/octet-stream, Size: 10348 bytes --]
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;
+ 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);
+ }
+ 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);
+}
+
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))
+ 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))
+ 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) {
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [linux-cifs-client][patch] utilize lookup intents to open in lookup
2009-04-01 19:33 ` Shirish Pargaonkar
@ 2009-04-09 12:55 ` Jeff Layton
2009-04-09 13:46 ` Shirish Pargaonkar
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2009-04-09 12:55 UTC (permalink / raw)
To: Shirish Pargaonkar
Cc: linux-cifs-client, linux-fsdevel, Steve French, viro, Trond Myklebust
On Wed, 1 Apr 2009 14:33:23 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [linux-cifs-client][patch] utilize lookup intents to open in lookup
2009-04-09 12:55 ` Jeff Layton
@ 2009-04-09 13:46 ` Shirish Pargaonkar
2009-04-09 14:00 ` [patch] " Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Shirish Pargaonkar @ 2009-04-09 13:46 UTC (permalink / raw)
To: Jeff Layton
Cc: linux-cifs-client, linux-fsdevel, Steve French, viro, Trond Myklebust
On Thu, Apr 9, 2009 at 7:55 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 1 Apr 2009 14:33:23 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> 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.
Jeff,
Thanks, will handle the identation/formatting errors in a subsequent patch
for sure.
If there is any other error code returned besides these two error codes,
it is handled as an error, so file pointer will not be instantiated
(as posix_open
will be true).
These two errors are there to handle the bug in samba posix open and
if either of these error codes are returned, cifs_lookup does what it
had been doing (as posix_open will be false),.
>
> 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@redhat.com>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch] utilize lookup intents to open in lookup
2009-04-09 13:46 ` Shirish Pargaonkar
@ 2009-04-09 14:00 ` Jeff Layton
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Layton @ 2009-04-09 14:00 UTC (permalink / raw)
To: Shirish Pargaonkar
Cc: linux-fsdevel, Steve French, linux-cifs-client, viro, Trond Myklebust
On Thu, 9 Apr 2009 08:46:07 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
> >> @@ -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.
>
> Jeff,
>
> Thanks, will handle the identation/formatting errors in a subsequent patch
> for sure.
>
> If there is any other error code returned besides these two error codes,
> it is handled as an error, so file pointer will not be instantiated
> (as posix_open
> will be true).
> These two errors are there to handle the bug in samba posix open and
> if either of these error codes are returned, cifs_lookup does what it
> had been doing (as posix_open will be false),.
>
Ok, thanks for clarifying that. Maybe a comment in there to explain
that would be helpful too?
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-04-09 14:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-31 16:57 [linux-cifs-client][patch] utilize lookup intents to open in lookup Shirish Pargaonkar
2009-04-01 19:33 ` Shirish Pargaonkar
2009-04-09 12:55 ` Jeff Layton
2009-04-09 13:46 ` Shirish Pargaonkar
2009-04-09 14:00 ` [patch] " Jeff Layton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.