[linux-cifs-client][patch] utilize lookup intents to open in
lookup
Shirish Pargaonkar
shirishpargaonkar at gmail.com
Thu Apr 9 13:46:07 GMT 2009
On Thu, Apr 9, 2009 at 7:55 AM, Jeff Layton <jlayton at redhat.com> wrote:
> On Wed, 1 Apr 2009 14:33:23 -0500
> Shirish Pargaonkar <shirishpargaonkar at 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 at redhat.com>
>
More information about the linux-cifs-client
mailing list