From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CCF2AC282C2 for ; Wed, 6 Feb 2019 13:37:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 99C4C20B1F for ; Wed, 6 Feb 2019 13:37:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728018AbfBFNh6 (ORCPT ); Wed, 6 Feb 2019 08:37:58 -0500 Received: from mail-wr1-f67.google.com ([209.85.221.67]:44509 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730275AbfBFNh6 (ORCPT ); Wed, 6 Feb 2019 08:37:58 -0500 Received: by mail-wr1-f67.google.com with SMTP id v16so5721257wrn.11 for ; Wed, 06 Feb 2019 05:37:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=badP3R5/Lu2yjL7Y492UhtV3cirXHZZNVj6rJ/FUb78=; b=P3ThBOIJXUpvaSkB95HnkooeHFEW/b/MjSMXaVW/MyfAUtxSrSztl11em3y9FMjv/S 2XKHCFuIreQ3oeu+k4GL81NMThyhRjxRL3BEvCOxwyZwrllfz80y5DPw5K/u/e6GKwol lLYDwGE4J3yy94eM/2HiPqqLIhnE6sqdh10395g5r+MS606/U9BusqT4jshRfO8AEaQX zytR/G+PD+X9artfuus/zcMGJ67RHkU2D2JI8LIvl+kUgF/MMQIeqovMZvgdo0ZhjOka MOrLLoqxdemZpQpgwgh4R05HGbTW7Rmn5fkvHt+TQqqw9GfZtc2WUTi+BrTrDT1xbeej 195g== X-Gm-Message-State: AHQUAuaL8o1b6+jiLj/Bl+WfkkEAa9zHsUpZTTTpxX4IdJI44nSwOptE 0K1i+SI7NBLdeNpWeu7Ol7KA2g== X-Google-Smtp-Source: AHgI3IZLN3CvfU8ioaHEcB3S8Frx2sdYIVZP5kDrSmKFMawoOm5xf9UXpkORlcIxCaDFgkAIoUBwnQ== X-Received: by 2002:adf:f347:: with SMTP id e7mr8019744wrp.25.1549460276596; Wed, 06 Feb 2019 05:37:56 -0800 (PST) Received: from hades.usersys.redhat.com (ip-89-103-126-188.net.upcbroadband.cz. [89.103.126.188]) by smtp.gmail.com with ESMTPSA id v22sm7427480wml.37.2019.02.06.05.37.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 06 Feb 2019 05:37:55 -0800 (PST) Date: Wed, 6 Feb 2019 14:37:53 +0100 From: Carlos Maiolino To: "Darrick J. Wong" Cc: linux-fsdevel@vger.kernel.org, hch@lst.de, adilger@dilger.ca, sandeen@redhat.com, david@fromorbit.com Subject: Re: [PATCH 09/10 V2] Use FIEMAP for FIBMAP calls Message-ID: <20190206133753.oqpw7citye6apdpr@hades.usersys.redhat.com> References: <20181205091728.29903-1-cmaiolino@redhat.com> <20181205091728.29903-10-cmaiolino@redhat.com> <20181205173650.GA8112@magnolia> <20190204151147.rra4n7k56ec4ndob@hades.usersys.redhat.com> <20190204182722.GA32119@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190204182722.GA32119@magnolia> User-Agent: NeoMutt/20180716 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org > > > In any case, I think a better solution to the multi-device problem is to > > > start returning device information via struct fiemap_extent, at least > > > inside the kernel. Use one of the reserved fields to declare a new > > > '__u32 fe_device' field in struct fiemap_extent which can be the dev_t > > > device number, and then you can check that against inode->i_sb->s_bdev > > > to avoid returning results for the non-primary device of a multi-device > > > filesystem. > > > > I agree we should address it here, but I don't think fiemap_extent is the right > > place for it, it is linked to the UAPI, and changing it is usually not a good > > idea. > > Adding a FIEMAP_EXTENT flag or two to turn one of the fe_reserved fields > into some sort of dev_t/per-device cookie should be fine. Userspace > shouldn't be expecting any meaning in reserved areas. > > > I think I got your idea anyway, but, what if, instead returning the bdev in > > fiemap_extent, we instead, send a flag (via fi_flags) to the filesystem, to > > idenfify a FIBMAP or a FIEMAP call, and let the filesystem decide what to do > > with such information? > > I don't like the idea of adding a FIEMAP_FLAG to distinguish callers. Ok, may I ask why not? My apologies if I am wrong, but, per my understanding, there is nothing today, which tells userspace which device belongs the extent map reported by FIEMAP. If it belongs to the RT device in XFS, or whatever disk in a raid in BTRFS, we simply do not provide such information. So, the goal is to provide a way to tell the filesystem if a FIEMAP or a FIBMAP has been requested, so the current behavior of both ioctls won't change. Enabling filesystems to return device information into fiemap_extent requires modification of all filesystems to provide such information, which will not have any use other than matching the mounted device to the device where the extent is. A FIEMAP_FLAG will also require FS changes, but IMHO, less intrusive than the device id in fiemap_extent. I don't see much advantage in adding the device id instead of using the flag. A problem I see using a new FIEMAP_FLAG, is it 'could' be also passed via userspace, so, it would require a check to make sure it didn't come from userspace if ioctl_fiemap() was used. I think there are 2 other possibilities which can be used to fix this. - Use a boolean inside fiemap_extent_info to identify a fibmap call, or, - If the device id is a must for you, maybe add the device id into fiemap_extent_info instead of fiemap_extent. So we don't mess with a UAPI exported data structure and still provides a way to the filesystems to provide which device the mapped extent is in. What you think? Cheers > > --D > > > > > > > > + > > > > + return error; > > > > +} > > > > + > > > > /** > > > > * bmap - find a block number in a file > > > > * @inode: inode owning the block number being requested > > > > @@ -1594,10 +1628,14 @@ EXPORT_SYMBOL(iput); > > > > */ > > > > int bmap(struct inode *inode, sector_t *block) > > > > { > > > > - if (!inode->i_mapping->a_ops->bmap) > > > > + if (inode->i_op->fiemap) > > > > + return bmap_fiemap(inode, block); > > > > + else if (inode->i_mapping->a_ops->bmap) > > > > + *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, > > > > + *block); > > > > + else > > > > return -EINVAL; > > > > > > Waitaminute. btrfs currently supports fiemap but not bmap, and now > > > suddenly it will support this legacy interface they've never supported > > > before. Are they on board with this? > > > > > > --D > > > > > > > > > > > - *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block); > > > > return 0; > > > > } > > > > EXPORT_SYMBOL(bmap); > > > > diff --git a/fs/ioctl.c b/fs/ioctl.c > > > > index 6086978fe01e..bfa59df332bf 100644 > > > > --- a/fs/ioctl.c > > > > +++ b/fs/ioctl.c > > > > @@ -116,6 +116,38 @@ int fiemap_fill_user_extent(struct fiemap_extent_info *fieinfo, u64 logical, > > > > return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > > > } > > > > > > > > +int fiemap_fill_kernel_extent(struct fiemap_extent_info *fieinfo, u64 logical, > > > > + u64 phys, u64 len, u32 flags) > > > > +{ > > > > + struct fiemap_extent *extent = fieinfo->fi_extents_start; > > > > + > > > > + /* only count the extents */ > > > > + if (fieinfo->fi_extents_max == 0) { > > > > + fieinfo->fi_extents_mapped++; > > > > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > > > + } > > > > + > > > > + if (fieinfo->fi_extents_mapped >= fieinfo->fi_extents_max) > > > > + return 1; > > > > + > > > > + if (flags & SET_UNKNOWN_FLAGS) > > > > + flags |= FIEMAP_EXTENT_UNKNOWN; > > > > + if (flags & SET_NO_UNMOUNTED_IO_FLAGS) > > > > + flags |= FIEMAP_EXTENT_ENCODED; > > > > + if (flags & SET_NOT_ALIGNED_FLAGS) > > > > + flags |= FIEMAP_EXTENT_NOT_ALIGNED; > > > > + > > > > + extent->fe_logical = logical; > > > > + extent->fe_physical = phys; > > > > + extent->fe_length = len; > > > > + extent->fe_flags = flags; > > > > + > > > > + fieinfo->fi_extents_mapped++; > > > > + > > > > + if (fieinfo->fi_extents_mapped == fieinfo->fi_extents_max) > > > > + return 1; > > > > + return (flags & FIEMAP_EXTENT_LAST) ? 1 : 0; > > > > +} > > > > /** > > > > * fiemap_fill_next_extent - Fiemap helper function > > > > * @fieinfo: Fiemap context passed into ->fiemap > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > > > index 7a434979201c..28bb523d532a 100644 > > > > --- a/include/linux/fs.h > > > > +++ b/include/linux/fs.h > > > > @@ -1711,6 +1711,8 @@ struct fiemap_extent_info { > > > > fiemap_fill_cb fi_cb; > > > > }; > > > > > > > > +int fiemap_fill_kernel_extent(struct fiemap_extent_info *info, u64 logical, > > > > + u64 phys, u64 len, u32 flags); > > > > int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical, > > > > u64 phys, u64 len, u32 flags); > > > > int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags); > > > > -- > > > > 2.17.2 > > > > > > > > -- > > Carlos -- Carlos