Jeff Layton | 3 Nov 15:07 2008
Picon

[PATCH] cifs: fix renaming one hardlink on top of another

POSIX says that renaming one hardlink on top of another to the same
inode is a no-op. We had the logic mostly right, but forgot to clear
the return code.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/cifs/inode.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index d54fa8a..ff8c68d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
 <at>  <at>  -1361,9 +1361,11  <at>  <at>  int cifs_rename(struct inode *source_dir, struct dentry *source_dentry,
 					CIFS_MOUNT_MAP_SPECIAL_CHR);

 		if (tmprc == 0 && (info_buf_source->UniqueId ==
-				   info_buf_target->UniqueId))
+				   info_buf_target->UniqueId)) {
 			/* same file, POSIX says that this is a noop */
+			rc = 0;
 			goto cifs_rename_exit;
+		}
 	} /* else ... BB we could add the same check for Windows by
 		     checking the UniqueId via FILE_INTERNAL_INFO */

--

-- 
1.5.5.1
Steve French | 3 Nov 16:12 2008
Picon

Re: [PATCH] cifs: fix renaming one hardlink on top of another

This fix looks right ... unfortunately the old code (2.6.27 and
earlier) had a similar bug, it looks like it returned EEXIST for this
case.

What are you using to test this - the mv command does not obey posix
semantics (does not call rename in vfs) and I am not convinced that
the rename command does either.

On Mon, Nov 3, 2008 at 8:07 AM, Jeff Layton <jlayton <at> redhat.com> wrote:
> POSIX says that renaming one hardlink on top of another to the same
> inode is a no-op. We had the logic mostly right, but forgot to clear
> the return code.
>
> Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
> ---
>  fs/cifs/inode.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index d54fa8a..ff8c68d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
>  <at>  <at>  -1361,9 +1361,11  <at>  <at>  int cifs_rename(struct inode *source_dir, struct dentry *source_dentry,
>                                        CIFS_MOUNT_MAP_SPECIAL_CHR);
>
>                if (tmprc == 0 && (info_buf_source->UniqueId ==
> -                                  info_buf_target->UniqueId))
> +                                  info_buf_target->UniqueId)) {
>                        /* same file, POSIX says that this is a noop */
> +                       rc = 0;
(Continue reading)

Jeff Layton | 3 Nov 16:23 2008
Picon

Re: Re: [PATCH] cifs: fix renaming one hardlink on top of another

On Mon, 3 Nov 2008 09:12:53 -0600
"Steve French" <smfrench <at> gmail.com> wrote:

> This fix looks right ... unfortunately the old code (2.6.27 and
> earlier) had a similar bug, it looks like it returned EEXIST for this
> case.
> 
> What are you using to test this - the mv command does not obey posix
> semantics (does not call rename in vfs) and I am not convinced that
> the rename command does either.
> 

I noticed it because the nfsidem test in connectathon failed (the
rename() failed with -EEXIST).

I don't think the older code failed this in most cases though. As long
as the last CIFSSMBUnixQPathInfo call worked, then we'll return 0 on
the old code in this case.

The reason this broke now is that we changed cifs_rename to use a tmprc
variable to hold the return code of the CIFSSMBUnixQPathInfo call in
order to preserve the value of "rc" while we're checking for a hardlink.

> On Mon, Nov 3, 2008 at 8:07 AM, Jeff Layton <jlayton <at> redhat.com> wrote:
> > POSIX says that renaming one hardlink on top of another to the same
> > inode is a no-op. We had the logic mostly right, but forgot to clear
> > the return code.
> >
> > Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
> > ---
(Continue reading)

Steve French | 3 Nov 16:27 2008
Picon

Re: Re: [PATCH] cifs: fix renaming one hardlink on top of another

On Mon, Nov 3, 2008 at 9:23 AM, Jeff Layton <jlayton <at> redhat.com> wrote:
> On Mon, 3 Nov 2008 09:12:53 -0600
> "Steve French" <smfrench <at> gmail.com> wrote:
>
>> This fix looks right ... unfortunately the old code (2.6.27 and
>> earlier) had a similar bug, it looks like it returned EEXIST for this
>> case.
>>
>> What are you using to test this - the mv command does not obey posix
>> semantics (does not call rename in vfs) and I am not convinced that
>> the rename command does either.
>>
>
> I noticed it because the nfsidem test in connectathon failed (the
> rename() failed with -EEXIST).
>
> I don't think the older code failed this in most cases though. As long
> as the last CIFSSMBUnixQPathInfo call worked, then we'll return 0 on
> the old code in this case.
>
> The reason this broke now is that we changed cifs_rename to use a tmprc
> variable to hold the return code of the CIFSSMBUnixQPathInfo call in

Yes - you are right, I missed that we changed how the rc (EEXIST) got
overwritten

We really need to build a simpler set of posix fs tests ... than the
connectathon special cases so we can make this testing easier to
read/debug.   Wish the ltp fs group was more active

(Continue reading)

Shirish Pargaonkar | 3 Nov 17:39 2008
Picon

[patch] CIFS command behaviour based on mid status

If the midStatus is 'retry needed' return with egain and do not attempt
reconnect as a reconnect just happened.
Attachment (midretry.patch): application/octet-stream, 1827 bytes
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client <at> lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Shirish Pargaonkar | 3 Nov 18:15 2008
Picon

[patch] assign correct values to iov after error in kernel_recvmsg

Can't rely on iov length and base being correct when kernel_recvmsg returns
an error when retrying kernel_recvmsg.
Attachment (recvmsg.patch): application/octet-stream, 984 bytes
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client <at> lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Shirish Pargaonkar | 3 Nov 19:23 2008
Picon

Re: [patch] CIFS command behaviour based on mid status

On Mon, Nov 3, 2008 at 10:39 AM, Shirish Pargaonkar
<shirishpargaonkar <at> gmail.com> wrote:
> If the midStatus is 'retry needed' return with egain and do not attempt
> reconnect as a reconnect just happened.
>

Another version of this patch, took out two redundant lines.

Regards,

Shirish
Attachment (midretry.1.patch): application/octet-stream, 1706 bytes
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client <at> lists.samba.org
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Steve French | 3 Nov 19:35 2008
Picon

Re: [PATCH] cifs: fix renaming one hardlink on top of another

Merged. thx

On Mon, Nov 3, 2008 at 8:07 AM, Jeff Layton <jlayton <at> redhat.com> wrote:
> POSIX says that renaming one hardlink on top of another to the same
> inode is a no-op. We had the logic mostly right, but forgot to clear
> the return code.
>
> Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
> ---
>  fs/cifs/inode.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index d54fa8a..ff8c68d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
>  <at>  <at>  -1361,9 +1361,11  <at>  <at>  int cifs_rename(struct inode *source_dir, struct dentry *source_dentry,
>                                        CIFS_MOUNT_MAP_SPECIAL_CHR);
>
>                if (tmprc == 0 && (info_buf_source->UniqueId ==
> -                                  info_buf_target->UniqueId))
> +                                  info_buf_target->UniqueId)) {
>                        /* same file, POSIX says that this is a noop */
> +                       rc = 0;
>                        goto cifs_rename_exit;
> +               }
>        } /* else ... BB we could add the same check for Windows by
>                     checking the UniqueId via FILE_INTERNAL_INFO */
>
> --
(Continue reading)

Steve French | 3 Nov 19:59 2008
Picon

Re: [patch] CIFS command behaviour based on mid status

I don't think that this is right ... we won't change the socket status
to CifsNeedReconnect unless the request is in state
MID_REQUEST_SUBMITTED, so there should be no need for you change.

(and note that you changed the return code when we are shutting down
from EHOSTDOWN to EIO, which may be harmless but changes behavior)

On Mon, Nov 3, 2008 at 12:23 PM, Shirish Pargaonkar
<shirishpargaonkar <at> gmail.com> wrote:
> On Mon, Nov 3, 2008 at 10:39 AM, Shirish Pargaonkar
> <shirishpargaonkar <at> gmail.com> wrote:
>> If the midStatus is 'retry needed' return with egain and do not attempt
>> reconnect as a reconnect just happened.
>>
>
> Another version of this patch, took out two redundant lines.
>
> Regards,
>
> Shirish
>

--

-- 
Thanks,

Steve
Steve French | 3 Nov 20:11 2008
Picon

Re: [patch] assign correct values to iov after error in kernel_recvmsg

This doesn't look right ... smb_msg.,msg_control and msg_controllen
shouldn't be changed (if so could you verify with the linux net
mailing list).   Are you sure that the socket API corrupts the iov
(e.g. iov_len) on EAGAIN?

Did this patch cause a change in the test case?   It is probably
harmless since it sets the values to what I would expect ... but would
like to see if someone on the linux net mailing list could verify.

On Mon, Nov 3, 2008 at 11:46 AM, Shirish Pargaonkar
<shirishpargaonkar <at> gmail.com> wrote:
> ---------- Forwarded message ----------
> From: Shirish Pargaonkar <shirishpargaonkar <at> gmail.com>
> Date: Mon, Nov 3, 2008 at 11:15 AM
> Subject: [linux-cifs-client][patch] assign correct values to iov after
> error in kernel_recvmsg
> To: "linux-cifs-client <at> lists.samba.org" <linux-cifs-client <at> lists.samba.org>
>
>
> Can't rely on iov length and base being correct when kernel_recvmsg returns
> an error when retrying kernel_recvmsg.
>

--

-- 
Thanks,

Steve

Gmane