Bug 14790 - vfs_btrfs compression support broken
Summary: vfs_btrfs compression support broken
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.14.5
Hardware: All Linux
: P5 regression (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-08 16:51 UTC by Noel Maximilian Kuntze
Modified: 2021-09-13 13:46 UTC (History)
1 user (show)

See Also:


Attachments
Patch that reworks the logic in btrfs_fget_compression (5.81 KB, patch)
2021-08-08 16:51 UTC, Noel Maximilian Kuntze
no flags Details
Patch that reworks the logic in btrfs_fget_compression (right patch) (3.05 KB, patch)
2021-08-08 16:53 UTC, Noel Maximilian Kuntze
no flags Details
Possible patch for master (1.39 KB, patch)
2021-08-09 17:32 UTC, Ralph Böhme
no flags Details
git-am fix for 4.15.next, 4.14.next. (1.47 KB, patch)
2021-09-10 18:47 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Noel Maximilian Kuntze 2021-08-08 16:51:14 UTC
Created attachment 16722 [details]
Patch that reworks the logic in btrfs_fget_compression

Dear Sir/Madam,

We have discovered that with commit a55a2bfb9143002161ef119298891725c6d3c075
from 2020-10-13, the function btrfs_fget_compression is permanently broken.
That commit is the rewrite from SMB_VFS_GET_COMPRESSION to SMB_VFS_FGET_COMPRESSION.
It is the last commit that touches the btrfs_fget_compression function. There is none after it.

The issue is that if the branch in source3/modules/vfs_btrfs.c:466 is taken, the call to ioctl() will always fail.
That is because the fd is initialized to -1 and then never touched again.

Code from master:

-------------------------------
static NTSTATUS btrfs_fget_compression(struct vfs_handle_struct *handle,
                       TALLOC_CTX *mem_ctx,
                       struct files_struct *fsp,
                       uint16_t *_compression_fmt)
{
    char buf[PATH_MAX];
    const char *p = NULL;
    int ret;
    long flags = 0;
> int fd = -1;
    NTSTATUS status;

    if (!fsp->fsp_flags.is_pathref) {
>     ret = ioctl(fd, FS_IOC_GETFLAGS, &flags);
        if (ret < 0) {
            DBG_WARNING("FS_IOC_GETFLAGS failed: %s, fd %lld\n",
                    strerror(errno), (long long)fd);
            return map_nt_error_from_unix(errno);
        }
[...]
-------------------------------

This happpens always for any share using btrfs as filesystem.
This causes a logging of the error message in line 469 for every file request, which means it happens ever
couple of milliseconds.

This effectively breaks btrfs support. This pertains any release in the 4.14 series.

We have developed a small patch that reworks the logic to avoid calling ioctl on fd=-1, but the compression remains broken. The patch is attached. It applies onto 4.14.5.

Kind regards
Noel Kuntze
Comment 1 Noel Maximilian Kuntze 2021-08-08 16:53:12 UTC
Created attachment 16723 [details]
Patch that reworks the logic in btrfs_fget_compression (right patch)
Comment 2 Ralph Böhme 2021-08-08 17:16:27 UTC
Hm, look like I broke it, let me check...
Comment 3 Noel Maximilian Kuntze 2021-08-08 17:28:18 UTC
The patch effectively silences the bug, but all written files are not compressed. So I guess set_compression is broken, too.
Comment 4 Noel Maximilian Kuntze 2021-08-08 17:37:47 UTC
Correction: Not all files are written compressed anymore. Some are compressed, some aren't.
Comment 5 Ralph Böhme 2021-08-09 17:32:39 UTC
Created attachment 16725 [details]
Possible patch for master

Can you please check this version?
Comment 6 Noel Maximilian Kuntze 2021-08-24 08:36:19 UTC
Thank you. The patch works.
Comment 7 Klemen Mihevc 2021-08-28 13:38:00 UTC
Patch works here as well.
Comment 8 Noel Maximilian Kuntze 2021-08-31 14:55:18 UTC
When can we expect this patch to be merged into master?
Comment 9 Ralph Böhme 2021-08-31 15:09:32 UTC
(In reply to Noel Maximilian Kuntze from comment #8)
I've put it on my todo list.
Comment 10 Noel Maximilian Kuntze 2021-09-10 13:45:52 UTC
Is it normal that it takes so long to process a patch for a regression (Priority 5)?
Comment 11 Ralph Böhme 2021-09-10 13:50:27 UTC
(In reply to Noel Maximilian Kuntze from comment #10)
Usually not, no. But as "amount of work to do / available development resources = time it takes" sometimes specific bugs or feautures sit a wee bit longer in the queue. Sorry!
Comment 12 Noel Maximilian Kuntze 2021-09-10 14:25:24 UTC
But it's a regression on a complete feature in use, not a new feature? So what exactly happened here? It makes no sense to me.
Comment 13 Jeremy Allison 2021-09-10 16:37:30 UTC
(In reply to Noel Maximilian Kuntze from comment #12)

Noel, if you need a guaranteed response time for a bug, the only way to get that is to purchase a support contract. Without that, all bug fixes are done on a best effort basis.

That's the norm in FLOSS projects.
Comment 14 Samba QA Contact 2021-09-10 18:17:03 UTC
This bug was referenced in samba master:

ed35fce4fe48b1fa26854a7b4bb151b5c5fb6fc6
Comment 15 Jeremy Allison 2021-09-10 18:47:42 UTC
Created attachment 16803 [details]
git-am fix for 4.15.next, 4.14.next.

Cherry-picked from master.
Comment 16 Ralph Böhme 2021-09-10 19:00:06 UTC
Reassigning to Jule for inclusion in 4.14 and 4.15.
Comment 17 Jule Anger 2021-09-13 07:41:49 UTC
Pushed to autobuild-v4-{15,14}-test.
Comment 18 Samba QA Contact 2021-09-13 08:52:05 UTC
This bug was referenced in samba v4-15-test:

35d474c303004c1a2a82fcd9f8f87a7b28a85713
Comment 19 Samba QA Contact 2021-09-13 09:25:28 UTC
This bug was referenced in samba v4-14-test:

e9cbf386be77230e3c3d51b878953bf4afbf93ff
Comment 20 Jule Anger 2021-09-13 11:42:13 UTC
Closing out bug report.

Thanks!
Comment 21 Samba QA Contact 2021-09-13 13:46:45 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.0rc7):

35d474c303004c1a2a82fcd9f8f87a7b28a85713