[PATCH] Fix streams_xattr (and fruit) with kernel oplocks

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
Hi folks,

attached find a fix for bug 12791: when kernel oplocks are enabled any VFS
module that does an open() on the underlying base file when processing a client
"stream open" request is doomed to cause all sort of havoc as it may trigger a
kernel oplock break which triggers panics and errors in the oplock break
handlers and the async open handling.

The basic idea of the fix is to *not* open() the underlying file for stream
related ops if it's not really needed and can't be replaced with a path-name
based equivalent. Luckily, in streams_xattr and fruit all calls can be replaced
with path-name based equivalents -- problem solved.

As the open() implementation in the VFS module has to return *something* that is
not (int)-1 and is ideally unique per open (although that's probably not really
required, but read on) I've opted to return a pipe fd, this satisfies both
conditions.

The patchset has been running at a large customer site for 4 weeks and so far
the previous daily crashes are gone. It also just passed another private
autobuild.

Please review & push if happy. Thanks!

-slow

bug12791-master.patch (57K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
On Thu, Jul 27, 2017 at 11:15 AM, Ralph Böhme via samba-technical
<[hidden email]> wrote:

> Hi folks,
>
> attached find a fix for bug 12791: when kernel oplocks are enabled any VFS
> module that does an open() on the underlying base file when processing a client
> "stream open" request is doomed to cause all sort of havoc as it may trigger a
> kernel oplock break which triggers panics and errors in the oplock break
> handlers and the async open handling.
>
> The basic idea of the fix is to *not* open() the underlying file for stream
> related ops if it's not really needed and can't be replaced with a path-name
> based equivalent. Luckily, in streams_xattr and fruit all calls can be replaced
> with path-name based equivalents -- problem solved.
>
> As the open() implementation in the VFS module has to return *something* that is
> not (int)-1 and is ideally unique per open (although that's probably not really
> required, but read on) I've opted to return a pipe fd, this satisfies both
> conditions.
>
> The patchset has been running at a large customer site for 4 weeks and so far
> the previous daily crashes are gone. It also just passed another private
> autobuild.
>
> Please review & push if happy. Thanks!

I have read through the patch and it looks OK, although I will need to
read it carefully one more time before I can approve it.

However, did you consider O_PATH? Since this seems to be Linux
specific (kernel oplocks) that might have avoided the need for a pipe,
although I will have to check the kernel code to see of an open with
O_PATH will trigger a kernel oplock break. Any attempt to use an FD
opened with O_PATH for anything serious will result in EBADF ...

Also, there seems to be some funny space vs tab issue here:

+static int streams_xattr_fchown(vfs_handle_struct *handle, files_struct *fsp,
+                               uid_t uid, gid_t gid)
+{
+        struct stream_io *sio =
+               (struct stream_io *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
+
+       if (sio == NULL) {
+               return SMB_VFS_NEXT_FCHOWN(handle, fsp, uid, gid);
+       }
+
+       return 0;
+}
+

and all the others in vfs_streams_xattr.c. The struct stream_io like
seems to have spaces at the beginning while the rest have tabs.

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
Hey Richard!

On Thu, Jul 27, 2017 at 12:09:36PM -0700, Richard Sharpe wrote:
> I have read through the patch and it looks OK, although I will need to
> read it carefully one more time before I can approve it.

thanks for taking the stab! :)

> However, did you consider O_PATH?

No, and I don't want to at this point. :) You'd have to proove that using a fd
returned from pipe() where the other end is close doesn't have the same
effect. Plus I *know* this one can't trigger oplock breaks in the kernel... ;)

> Since this seems to be Linux
> specific (kernel oplocks) that might have avoided the need for a pipe,
> although I will have to check the kernel code to see of an open with
> O_PATH will trigger a kernel oplock break. Any attempt to use an FD
> opened with O_PATH for anything serious will result in EBADF ...
>
> Also, there seems to be some funny space vs tab issue here:
>
> +static int streams_xattr_fchown(vfs_handle_struct *handle, files_struct *fsp,
> +                               uid_t uid, gid_t gid)
> +{
> +        struct stream_io *sio =
> +               (struct stream_io *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
> +
> +       if (sio == NULL) {
> +               return SMB_VFS_NEXT_FCHOWN(handle, fsp, uid, gid);
> +       }
> +
> +       return 0;
> +}
> +
>
> and all the others in vfs_streams_xattr.c. The struct stream_io like
> seems to have spaces at the beginning while the rest have tabs.
oh, didn't notice, thanks! Looks like I inherited this when copying the lines
from the fallocate function directly above. :)

Updated patchset attached.

Thanks!
-slow

bug12791-master.patch (57K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, Jul 27, 2017 at 11:15 AM, Ralph Böhme via samba-technical
<[hidden email]> wrote:

> Hi folks,
>
> attached find a fix for bug 12791: when kernel oplocks are enabled any VFS
> module that does an open() on the underlying base file when processing a client
> "stream open" request is doomed to cause all sort of havoc as it may trigger a
> kernel oplock break which triggers panics and errors in the oplock break
> handlers and the async open handling.
>
> The basic idea of the fix is to *not* open() the underlying file for stream
> related ops if it's not really needed and can't be replaced with a path-name
> based equivalent. Luckily, in streams_xattr and fruit all calls can be replaced
> with path-name based equivalents -- problem solved.
>
> As the open() implementation in the VFS module has to return *something* that is
> not (int)-1 and is ideally unique per open (although that's probably not really
> required, but read on) I've opted to return a pipe fd, this satisfies both
> conditions.
>
> The patchset has been running at a large customer site for 4 weeks and so far
> the previous daily crashes are gone. It also just passed another private
> autobuild.

Reviewing the second patch set.

First patch: RB+
Second patch:

I have one cosmetic suggestion with respect to this:

--- a/source3/modules/vfs_streams_xattr.c
+++ b/source3/modules/vfs_streams_xattr.c
@@ -232,8 +232,6 @@ static int streams_xattr_fstat(vfs_handle_struct
*handle, files_struct *fsp,
        struct stream_io *io = (struct stream_io *)
                VFS_FETCH_FSP_EXTENSION(handle, fsp);

-       DEBUG(10, ("streams_xattr_fstat called for %d\n", fsp->fh->fd));
-
        if (io == NULL || fsp->base_fsp == NULL) {
                return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
        }
@@ -242,6 +240,8 @@ static int streams_xattr_fstat(vfs_handle_struct
*handle, files_struct *fsp,
                return -1;
        }

+       DBG_DEBUG("streams_xattr_fstat called for %s\n", fsp_str_dbg(io->fsp));
+
        /* Create an smb_filename with stream_name == NULL. */
        smb_fname_base = synthetic_smb_fname(talloc_tos(),
                                        io->base,

I agree with what you are doing, but I suggest you move the new
DBG_DEBUG to the original position and use fsp->fsp_name. This is
because there are two early exits from that function and if they are
taken we will have no idea that this function was called.

Otherwise: Second patch RB+

Third patch: RB+
Fourth patch: RB+

Fifth patch. I have a question. AFAIK, there is only one DACL/SD for
the whole files, so in streams_xattr_sys_acl_blob_get_fd, shouldn't it
be fsp->base_fsp->fsp_name here?

+       return SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FILE(
+               handle, fsp->fsp_name, mem_ctx,
+               blob_description, blob);
+}

Otherwise Fifth paths: Would like someone else to review the stuff in
streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
weak.

Sixth patch (the one with the pipes): RB+
Seventh patch: RB+
Eighth patch: RB+
Ninth patch: RB+
Tenth patch: ad_get and ad_fget seem to have duplicated code. A
separate function might be useful although there are minor
differences.

Otherwise: Tenth patch: RB+

Eleventh patch: RB+
Christmas patch: RB+

--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
Hi

On Thu, Jul 27, 2017 at 01:38:22PM -0700, Richard Sharpe wrote:

> Reviewing the second patch set.
>
> First patch: RB+
> Second patch:
>
> I have one cosmetic suggestion with respect to this:
>
> --- a/source3/modules/vfs_streams_xattr.c
> +++ b/source3/modules/vfs_streams_xattr.c
> @@ -232,8 +232,6 @@ static int streams_xattr_fstat(vfs_handle_struct
> *handle, files_struct *fsp,
>         struct stream_io *io = (struct stream_io *)
>                 VFS_FETCH_FSP_EXTENSION(handle, fsp);
>
> -       DEBUG(10, ("streams_xattr_fstat called for %d\n", fsp->fh->fd));
> -
>         if (io == NULL || fsp->base_fsp == NULL) {
>                 return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
>         }
> @@ -242,6 +240,8 @@ static int streams_xattr_fstat(vfs_handle_struct
> *handle, files_struct *fsp,
>                 return -1;
>         }
>
> +       DBG_DEBUG("streams_xattr_fstat called for %s\n", fsp_str_dbg(io->fsp));
> +
>         /* Create an smb_filename with stream_name == NULL. */
>         smb_fname_base = synthetic_smb_fname(talloc_tos(),
>                                         io->base,
>
> I agree with what you are doing, but I suggest you move the new
> DBG_DEBUG to the original position and use fsp->fsp_name. This is
> because there are two early exits from that function and if they are
> taken we will have no idea that this function was called.
Good point. Fixed in the attached patchset.

> Fifth patch. I have a question. AFAIK, there is only one DACL/SD for
> the whole files, so in streams_xattr_sys_acl_blob_get_fd, shouldn't it
> be fsp->base_fsp->fsp_name here?
>
> +       return SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FILE(
> +               handle, fsp->fsp_name, mem_ctx,
> +               blob_description, blob);
> +}

Good catch, thanks! Guess that mistake crept in in the rebase on current master
when adopting to the recent VFS changes from Jeremy. Fixed.

> Otherwise Fifth paths: Would like someone else to review the stuff in
> streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
> weak.

This one is marked at TODO in the attached patchset, can someone please take a
look?

> Sixth patch (the one with the pipes): RB+
> Seventh patch: RB+
> Eighth patch: RB+
> Ninth patch: RB+
> Tenth patch: ad_get and ad_fget seem to have duplicated code. A
> separate function might be useful although there are minor
> differences.

Hey, it's a Christmas patchset after all, so your wishes came true, look at
patch no 12... :)

> Otherwise: Tenth patch: RB+
>
> Eleventh patch: RB+
> Christmas patch: RB+

Marry patchmess! :)

-slow

bug12791-master.patch (57K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
On Fri, Jul 28, 2017 at 08:04:12AM +0200, Ralph Böhme via samba-technical wrote:
> > Otherwise Fifth paths: Would like someone else to review the stuff in
> > streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
> > weak.
>
> This one is marked at TODO in the attached patchset, can someone please take a
> look?

can anyone take a look please? Thanks!

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
On Mon, Jul 31, 2017 at 05:19:42PM +0200, Ralph Böhme wrote:
> On Fri, Jul 28, 2017 at 08:04:12AM +0200, Ralph Böhme via samba-technical wrote:
> > > Otherwise Fifth paths: Would like someone else to review the stuff in
> > > streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
> > > weak.
> >
> > This one is marked at TODO in the attached patchset, can someone please take a
> > look?
>
> can anyone take a look please? Thanks!

hey, this one is starting to bitrot. :) Anyone?

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Fix streams_xattr (and fruit) with kernel oplocks

Samba - samba-technical mailing list
On Wed, Aug 09, 2017 at 03:27:42PM +0200, Ralph Böhme wrote:

> On Mon, Jul 31, 2017 at 05:19:42PM +0200, Ralph Böhme wrote:
> > On Fri, Jul 28, 2017 at 08:04:12AM +0200, Ralph Böhme via samba-technical wrote:
> > > > Otherwise Fifth paths: Would like someone else to review the stuff in
> > > > streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
> > > > weak.
> > >
> > > This one is marked at TODO in the attached patchset, can someone please take a
> > > look?
> >
> > can anyone take a look please? Thanks!
>
> hey, this one is starting to bitrot. :) Anyone?

Looking at it *RIGHT NOW*. Sorry.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Loading...