linux-kernel - Re: [GIT PULL] vfs netfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wjr8fxk20-wx=63mZruW1LTvBvAKya1GQ1EhyzXb-okMA@mail.gmail.com>
Date: Mon, 16 Sep 2024 12:28:34 +0200
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Christian Brauner <brauner@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	David Howells <dhowells@...hat.com>, Steve French <stfrench@...rosoft.com>
Subject: Re: [GIT PULL] vfs netfs

On Fri, 13 Sept 2024 at 18:57, Christian Brauner <brauner@...nel.org> wrote:
>
> /* Conflicts */
>
> Merge conflicts with mainline

Hmm.

My conflict resolution is _similar_, but at the same time decidedly
different. And I'm not sure why yours is different.

> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@@ -1261,16 -1261,6 +1261,14 @@@ openRetry
>         return rc;
>   }
>
>  +static void cifs_readv_worker(struct work_struct *work)
>  +{
>  +      struct cifs_io_subrequest *rdata =
>  +              container_of(work, struct cifs_io_subrequest, subreq.work);
>  +
> -       netfs_subreq_terminated(&rdata->subreq,
> -                               (rdata->result == 0 || rdata->result == -EAGAIN) ?
> -                               rdata->got_bytes : rdata->result, true);
> ++      netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false);

So here, I have

++      netfs_read_subreq_terminated(&rdata->subreq, rdata->result, true);

with the third argument being 'true' instead of 'false' as in yours.

The reason? That's what commit a68c74865f51 ("cifs: Fix SMB1
readv/writev callback in the same way as SMB2/3") did when it moved
the (originally) netfs_subreq_terminated() into the worker, and it
changed the 'was_async' argument from "false" to a "true".

Now, that change makes little sense to me (a worker thread is not
softirq context), but  that's what commit a68c74865f51 does, and so
that's logically what the merge should do.

> +       rdata->subreq.transferred += rdata->got_bytes;
>  -      netfs_read_subreq_terminated(&rdata->subreq, rdata->result, false);
> ++      trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);

where did this trace_netfs_sreq() come from?

> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@@ -4614,6 -4613,10 +4613,8 @@@ smb2_readv_callback(struct mid_q_entry
>                               server->credits, server->in_flight,
>                               0, cifs_trace_rw_credits_read_response_clear);
>         rdata->credits.value = 0;
> +       rdata->subreq.transferred += rdata->got_bytes;
>  -      if (rdata->subreq.start + rdata->subreq.transferred >= rdata->subreq.rreq->i_size)
>  -              __set_bit(NETFS_SREQ_HIT_EOF, &rdata->subreq.flags);
> +       trace_netfs_sreq(&rdata->subreq, netfs_sreq_trace_io_progress);

And where did this conflict resolution come from? I'm not seeing why
it removes that NETFS_SREQ_HIT_EOF bit logic..

Some searching gets me this:

    https://lore.kernel.org/all/1131388.1726141806@warthog.procyon.org.uk/

which seems to explain your merge resolution, and I'm even inclined to
think that it might be right, but it's not *sensible*.

That whole removal of the NETFS_SREQ_HIT_EOF bit ends up undoing part
of commit ee4cdf7ba857 ("netfs: Speed up buffered reading"), and
doesn't seem to be supported by the changes done on the other side of
the conflict resolution.

IOW, the changes may be *correct*, but they do not seem to be valid as
a conflict resolution, and if they are fixes they should be done as
such separately.

Adding DavidH (and Steve French) to the participants, so that they can
know about my confusion, and maybe send a patch to fix it up properly
with actual explanations. Because I don't want to commit the merge as
you suggest without explanations for why those changes were magically
done independently of what seems to be happening in the development
history.

                 Linus