LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Gao Xiang <gaoxiang25@huawei.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: devel@driverdev.osuosl.org,
	"Sasha Levin" <alexander.levin@microsoft.com>,
	"Valdis Klētnieks" <valdis.kletnieks@vt.edu>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	"Christoph Hellwig" <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org,
	"OGAWA Hirofumi" <hirofumi@mail.parknet.co.jp>
Subject: Re: [PATCH] staging: exfat: add exfat filesystem code to staging
Date: Fri, 30 Aug 2019 00:04:41 +0800
Message-ID: <20190829160441.GA141079@architecture4> (raw)
In-Reply-To: <20190829155127.GA136563@architecture4>

Hi Dan,

On Thu, Aug 29, 2019 at 11:51:27PM +0800, Gao Xiang wrote:
> Hi Dan,
> 
> On Thu, Aug 29, 2019 at 06:43:46PM +0300, Dan Carpenter wrote:
> > > p.s. There are 2947 (un)likely places in fs/ directory.
> > 
> > I was complaining about you adding new pointless ones, not existing
> > ones.  The likely/unlikely annotations are supposed to be functional and
> > not decorative.  I explained this very clearly.
> 
> I don't think that is mostly pointless. I think it has functional use
> because all error handling paths are rarely happened, or can you remove
> the unlikely in IS_ERR as well?
> 
> > 
> > Probably most of the annotations in fs/ are wrong but they are also
> > harmless except for the slight messiness.  However there are definitely
> > some which are important so removing them all isn't a good idea.
> > 
> > > If you like, I will delete them all.
> > 
> > But for erofs, I don't think that any of the likely/unlikely calls have
> > been thought about so I'm fine with removing all of them in one go.
> 
> Maybe some misuse but rare, I will show you case by case. Wait a minute.

Anyway, I'm fine to delete them all if you like, but I think majority of these
are meaningful.

data.c-		/* page is already locked */
data.c-		DBG_BUGON(PageUptodate(page));
data.c-
data.c:		if (unlikely(err))
data.c-			SetPageError(page);
data.c-		else
data.c-			SetPageUptodate(page);
--
IO error

data.c-
data.c-repeat:
data.c-	page = find_or_create_page(mapping, blkaddr, gfp);
data.c:	if (unlikely(!page)) {
data.c-		DBG_BUGON(nofail);
data.c-		return ERR_PTR(-ENOMEM);
data.c-	}
--
NO MEM


data.c-		}
data.c-
data.c-		err = bio_add_page(bio, page, PAGE_SIZE, 0);
data.c:		if (unlikely(err != PAGE_SIZE)) {
data.c-			err = -EFAULT;
data.c-			goto err_out;

Internal error (since it is a single bio with one page).

data.c-		}
---

data.c-		lock_page(page);
data.c-
data.c-		/* this page has been truncated by others */
data.c:		if (unlikely(page->mapping != mapping)) {
data.c-unlock_repeat:
data.c-			unlock_page(page);
data.c-			put_page(page);
data.c-			goto repeat;
data.c-		}

truncated

data.c-
data.c:		/* more likely a read error */
data.c:		if (unlikely(!PageUptodate(page))) {
data.c-			if (io_retries) {
data.c-				--io_retries;
data.c-				goto unlock_repeat;

IO err

--
data.c-	nblocks = DIV_ROUND_UP(inode->i_size, PAGE_SIZE);
data.c-	lastblk = nblocks - is_inode_flat_inline(inode);
data.c-
data.c:	if (unlikely(offset >= inode->i_size)) {
data.c-		/* leave out-of-bound access unmapped */
data.c-		map->m_flags = 0;
data.c-		map->m_plen = 0;
--

FS corrupted

data.c-int erofs_map_blocks(struct inode *inode,
data.c-		     struct erofs_map_blocks *map, int flags)
data.c-{
data.c:	if (unlikely(is_inode_layout_compression(inode))) {

for compressed files, should call z_erofs_map_blocks_iter since it
behaves in iterative way. but I think that can be deleted.

data.c-		int err = z_erofs_map_blocks_iter(inode, map, flags);
data.c-
data.c-		if (map->mpage) {
--
data.c-		unsigned int blkoff;
data.c-
data.c-		err = erofs_map_blocks(inode, &map, EROFS_GET_BLOCKS_RAW);
data.c:		if (unlikely(err))
data.c-			goto err_out;

Error

data.c-
data.c-		/* zero out the holed page */
data.c:		if (unlikely(!(map.m_flags & EROFS_MAP_MAPPED))) {

no hole in erofs.

data.c-			zero_user_segment(page, 0, PAGE_SIZE);
data.c-			SetPageUptodate(page);
data.c-
--
data.c-submit_bio_out:
data.c-		__submit_bio(bio, REQ_OP_READ, 0);
data.c-
data.c:	return unlikely(err) ? ERR_PTR(err) : NULL;

err

data.c-}
data.c-
data.c-/*
--
data.c-	DBG_BUGON(!list_empty(pages));
data.c-
data.c-	/* the rare case (end in gaps) */
data.c:	if (unlikely(bio))
data.c-		__submit_bio(bio, REQ_OP_READ, 0);
data.c-	return 0;
data.c-}


decompressor.c-			get_page(victim);
decompressor.c-		} else {
decompressor.c-			victim = erofs_allocpage(pagepool, GFP_KERNEL, false);
decompressor.c:			if (unlikely(!victim))
decompressor.c-				return -ENOMEM;

nomem

decompressor.c-			victim->mapping = Z_EROFS_MAPPING_STAGING;
decompressor.c-		}


dir.c-			de_namelen = le16_to_cpu(de[1].nameoff) - nameoff;
dir.c-
dir.c-		/* a corrupted entry is found */
dir.c:		if (unlikely(nameoff + de_namelen > maxsize ||
dir.c-			     de_namelen > EROFS_NAME_LEN)) {

corrupted

dir.c-			errln("bogus dirent @ nid %llu", EROFS_V(dir)->nid);
dir.c-			DBG_BUGON(1);
--
dir.c-
dir.c-		nameoff = le16_to_cpu(de->nameoff);
dir.c-
dir.c:		if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
dir.c-			     nameoff >= PAGE_SIZE)) {

corrupted

dir.c-			errln("%s, invalid de[0].nameoff %u @ nid %llu",
dir.c-			      __func__, nameoff, EROFS_V(dir)->nid);
--
dir.c-				dirsize - ctx->pos + ofs, PAGE_SIZE);
dir.c-
dir.c-		/* search dirents at the arbitrary position */
dir.c:		if (unlikely(initial)) {

as comments said

dir.c-			initial = false;
dir.c-
dir.c-			ofs = roundup(ofs, sizeof(struct erofs_dirent));
dir.c:			if (unlikely(ofs >= nameoff))
dir.c-				goto skip_this;
dir.c-		}
dir.c-
--
dir.c-
dir.c-		ctx->pos = blknr_to_addr(i) + ofs;
dir.c-
dir.c:		if (unlikely(err))

err

dir.c-			break;
dir.c-		++i;
dir.c-		ofs = 0;

inode.c-
inode.c-	vi->datamode = __inode_data_mapping(advise);
inode.c-
inode.c:	if (unlikely(vi->datamode >= EROFS_INODE_LAYOUT_MAX)) {
inode.c-		errln("unsupported data mapping %u of nid %llu",
inode.c-		      vi->datamode, vi->nid);
inode.c-		DBG_BUGON(1);

err

--
inode.c-	if (S_ISLNK(inode->i_mode) && inode->i_size < PAGE_SIZE) {
inode.c-		char *lnk = erofs_kmalloc(sbi, inode->i_size + 1, GFP_KERNEL);
inode.c-
inode.c:		if (unlikely(!lnk))
inode.c-			return -ENOMEM;
inode.c-
inode.c-		m_pofs += vi->inode_isize + vi->xattr_isize;
inode.c-
inode.c-		/* inline symlink data shouldn't across page boundary as well */
inode.c:		if (unlikely(m_pofs + inode->i_size > PAGE_SIZE)) {
inode.c-			kfree(lnk);
inode.c-			errln("inline data cross block boundary @ nid %llu",
inode.c-			      vi->nid);

err

--
inode.c-{
inode.c-	struct inode *inode = erofs_iget_locked(sb, nid);
inode.c-
inode.c:	if (unlikely(!inode))
inode.c-		return ERR_PTR(-ENOMEM);

err

inode.c-
inode.c-	if (inode->i_state & I_NEW) {
--
inode.c-		vi->nid = nid;
inode.c-
inode.c-		err = fill_inode(inode, isdir);
inode.c:		if (likely(!err))
inode.c-			unlock_new_inode(inode);
inode.c-		else {

err

inode.c-			iget_failed(inode);

I will stop here.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
> > 
> > regards,
> > dan carpenter
> > 

  reply index

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-28 16:08 Greg Kroah-Hartman
2019-08-28 17:00 ` Greg Kroah-Hartman
2019-08-29  6:23   ` Christoph Hellwig
2019-08-29  6:39     ` Greg Kroah-Hartman
2019-08-29  9:41       ` Christoph Hellwig
2019-08-29  9:50         ` Greg Kroah-Hartman
2019-08-29 10:37           ` Christoph Hellwig
2019-08-29 11:04             ` Gao Xiang
2019-08-29 11:18               ` Greg Kroah-Hartman
2019-08-29 11:18             ` Greg Kroah-Hartman
2019-08-29 15:11               ` Dan Carpenter
2019-08-29 15:27                 ` Gao Xiang
2019-08-29 15:43                   ` Dan Carpenter
2019-08-29 15:51                     ` Gao Xiang
2019-08-29 16:04                       ` Gao Xiang [this message]
2019-08-30  8:34                         ` Dan Carpenter
2019-08-30  8:43                           ` Gao Xiang
2019-08-30 11:26                             ` Dan Carpenter
2019-08-30 12:04                               ` Gao Xiang
2019-08-29 16:44                     ` Gao Xiang
2019-08-29 16:59                       ` Joe Perches
2019-08-29 17:02                         ` Gao Xiang
2019-08-30  2:06                     ` Chao Yu
2019-08-30  6:38                       ` Gao Xiang
2019-08-30 12:00                         ` Checking usage of likeliness annotations Markus Elfring
2019-08-30 11:51                       ` [PATCH] staging: exfat: add exfat filesystem code to staging David Sterba
2019-08-31  3:50                         ` Chao Yu
2019-08-30 15:36               ` Christoph Hellwig
2019-08-30 21:54               ` Dave Chinner
2019-08-31 10:31                 ` Valdis Klētnieks
2019-09-01  0:04                   ` Dave Chinner
2019-08-29  7:01     ` Gao Xiang
2019-08-29  8:24       ` Gao Xiang
2019-08-29  9:51         ` Christoph Hellwig
2019-08-29 12:14 ` Pali Rohár
2019-08-29 12:34   ` Valdis Klētnieks
2019-08-29 12:46     ` Pali Rohár
2019-08-29 14:08 ` Markus Elfring
2019-08-29 15:44 ` Markus Elfring
2019-08-29 20:56 ` Pali Rohár
2019-08-29 23:18   ` Valdis Klētnieks
2019-08-29 23:35     ` Sasha Levin
2019-08-30  7:56       ` Pali Rohár
2019-10-16 14:03         ` Pali Rohár
2019-10-16 14:31           ` Sasha Levin
2019-10-16 16:03             ` Pali Rohár
2019-10-16 16:20               ` Sasha Levin
2019-10-16 16:22               ` Greg Kroah-Hartman
2019-10-16 16:32                 ` Pali Rohár
2019-10-16 16:50                   ` Greg Kroah-Hartman
2019-10-16 20:33               ` Sasha Levin
2019-10-16 21:53                 ` Valdis Klētnieks
2019-10-17  7:53                   ` Pali Rohár
2019-10-17  7:50                 ` Pali Rohár
2019-10-16 16:05             ` Valdis Klētnieks
2019-08-30  8:03     ` Pali Rohár
2019-08-30 15:40   ` Christoph Hellwig
2019-08-30 15:43     ` Pali Rohár
2019-09-14 13:39 ` [PATCH] staging: exfat: add exfat filesystem code to Park Ju Hyung
2019-09-15 13:54   ` Greg KH
2019-09-15 16:11     ` Ju Hyung Park
     [not found] ` <20190918195920.25210-1-qkrwngud825@gmail.com>
2019-09-18 20:12   ` [PATCH] staging: exfat: rebase to sdFAT v2.2.0 Greg KH
2019-09-18 20:13   ` Greg KH
2019-09-18 20:22     ` Ju Hyung Park
2019-09-18 20:26       ` Greg KH
2019-09-18 20:31         ` Ju Hyung Park
2019-09-18 20:46           ` Valdis Klētnieks
2019-09-18 21:31   ` kbuild test robot
2019-09-18 21:31   ` kbuild test robot
2019-09-18 22:17     ` Ju Hyung Park

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190829160441.GA141079@architecture4 \
    --to=gaoxiang25@huawei.com \
    --cc=alexander.levin@microsoft.com \
    --cc=dan.carpenter@oracle.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=hirofumi@mail.parknet.co.jp \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=valdis.kletnieks@vt.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox