From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:51325 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752055AbcCGKSN (ORCPT ); Mon, 7 Mar 2016 05:18:13 -0500 Date: Mon, 7 Mar 2016 11:18:35 +0100 From: Jan Kara To: Bob Peterson Cc: linux-fsdevel@vger.kernel.org, Jan Kara , Al Viro , Dave Chinner , Christoph Hellwig Subject: Re: [vfs PATCH v3 2/4] VFS: Add new __generic_iomap_fiemap interface Message-ID: <20160307101835.GE5201@quack.suse.cz> References: <1457122300-28514-1-git-send-email-rpeterso@redhat.com> <1457122300-28514-3-git-send-email-rpeterso@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1457122300-28514-3-git-send-email-rpeterso@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri 04-03-16 15:11:38, Bob Peterson wrote: > @@ -251,6 +253,100 @@ static inline loff_t blk_to_logical(struct inode *inode, sector_t blk) > } > > /** > + * __generic_iomap_fiemap - FIEMAP for iomap based inodes (no locking) > + * @inode: the inode to map > + * @fieinfo: the fiemap info struct that will be passed back to userspace > + * @start: where to start mapping in the inode > + * @len: how much space to map You are missing @iomap description here. > + * > + * This does FIEMAP for iomap based inodes. Basically it will just loop > + * through iomap until we hit the number of extents we want to map, or we > + * go past the end of the file and hit a hole. > + * > + * If it is possible to have data blocks beyond a hole past @inode->i_size, then > + * please do not use this function, it will stop at the first unmapped block > + * beyond i_size. Line over 80 chars here. > + * > + * If you use this function directly, you need to do your own locking. Use > + * generic_iomap_fiemap if you want the locking done for you. > + */ > + > +int __generic_iomap_fiemap(struct inode *inode, > + struct fiemap_extent_info *fieinfo, loff_t start, > + loff_t len, iomap_get_t iomap) > +{ > + struct iomap iom, prev_iom; > + loff_t isize = i_size_read(inode); > + int ret = 0; > + > + ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC); > + memset(&prev_iom, 0, sizeof(prev_iom)); > + if (len >= isize) > + len = isize; > + > + while ((ret == 0) && (start < isize) && len) { > + memset(&iom, 0, sizeof(iom)); > + ret = iomap(inode->i_mapping, start, len, &iom); > + if (ret) > + break; > + > + if (!iomap_needs_allocation(&iom)) { Why not directly check for hole here? The name iomap_needs_allocation() only obscures what's going on here. At least to me... ;) > + if (prev_iom.blkno) > + ret = fiemap_fill_next_extent(fieinfo, > + prev_iom.offset, > + blk_to_logical(inode, > + prev_iom.blkno), > + prev_iom.length, > + FIEMAP_EXTENT_MERGED); I'd just format this as ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset, blk_to_logical(inode, prev_iom.blkno), prev_iom.length, FIEMAP_EXTENT_MERGED); IMHO it is more readable and doesn't overflow 80 chars. > + prev_iom = iom; > + } > + start += iom.length; > + if (len < iom.length) > + break; > + len -= iom.length; > + cond_resched(); Add the fatal_signal_pending() check we have in __generic_block_fiemap()? > + } > + > + if (prev_iom.blkno) > + ret = fiemap_fill_next_extent(fieinfo, prev_iom.offset, > + blk_to_logical(inode, > + prev_iom.blkno), > + prev_iom.length, > + FIEMAP_EXTENT_MERGED | > + FIEMAP_EXTENT_LAST); FIEMAP_EXTENT_LAST should only be set if this is the last extent in the file. Here you set it in other cases as well. Also in some cases - e.g. when fiemap_fill_next_extent() in the loop above runs out of space in fieinfo, or in case of error, you shouldn't try to fill next extent, should you? > typedef int (get_block_t)(struct inode *inode, sector_t iblock, > struct buffer_head *bh_result, int create); > +typedef int (iomap_get_t)(struct address_space *mapping, loff_t pos, > + ssize_t length, struct iomap *iomap); Why do you pass 'struct address_space' and not 'struct inode'? That would look more natural to me... Honza -- Jan Kara SUSE Labs, CR