From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com ([209.85.221.68]:44352 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729160AbeKFSgH (ORCPT ); Tue, 6 Nov 2018 13:36:07 -0500 Received: by mail-wr1-f68.google.com with SMTP id j17-v6so7412012wrq.11 for ; Tue, 06 Nov 2018 01:11:51 -0800 (PST) Date: Tue, 6 Nov 2018 10:11:48 +0100 From: Carlos Maiolino To: Andreas Dilger Cc: Linux FS-devel Mailing List , Eric Sandeen , hch@lst.de, david@fromorbit.com, darrick.wong@oracle.com Subject: Re: [PATCH 18/20] Use FIEMAP for FIBMAP calls Message-ID: <20181106091147.zbkukzro3k63y7gj@odin.usersys.redhat.com> References: <20181030131823.29040-1-cmaiolino@redhat.com> <20181030131823.29040-19-cmaiolino@redhat.com> <87C8F7EE-519A-4C36-A56C-D9B5E9814D8C@dilger.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87C8F7EE-519A-4C36-A56C-D9B5E9814D8C@dilger.ca> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: > > + if (inode->i_op->fiemap) { > > + fextent.fe_logical = 0; > > + fextent.fe_physical = 0; > > + f_ctx.fc_extents_max = 1; > > + f_ctx.fc_extents_mapped = 0; > > + f_ctx.fc_data = &fextent; > > + f_ctx.fc_start = start; > > + f_ctx.fc_len = 1; > > Should this be "fc_len = 1 << inode->i_blkbits" to map a single block? D'oh, yeah, I just re-read fiemap implementation, and yeah, len is supposed to be byte-sized, not block sized, so you are right. It doesn't really make much difference here, due the purpose of the fiemap call here, but still it's wrong. I'll update it on a future review. > On the one hand, this might return multiple blocks, but on the other > hand FIBMAP shouldn't be allowed if that is the case so this code should > detect that situation and consider it an error. Well, FIEMAP interface is used internally to get the block mapping requested by the user via FIBMAP, so, yes, FIEMAP interface will return the whole extent, but to the user, the ioctl will return just the mapping of the requested block. Which is basically what we do here: > > + *block = (fextent.fe_physical + > > + (start - fextent.fe_logical)) >> inode->i_blkbits; > > + We only return a single address back, not the whole extent map. Again, thanks for the review, much appreciated -- Carlos