| Message ID | 20171012231500.19489-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jaegeuk, On 2017/10/13 7:15, Jaegeuk Kim wrote: > This patch returns an error number to quota_write in order for quota to handle > it correctly. We should return error number like __generic_file_write_iter, right? it needs to return written bytes if we have written one page or more, otherwise return error number feedbacked from write_begin. So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on f2fs_quota_write")? Thanks, > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > fs/f2fs/super.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 2feecf5e7f4c..840a0876005b 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, > > err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, > &page, NULL); > - if (unlikely(err)) > + if (unlikely(err)) { > + if (len == towrite) > + return err; > break; > + } > > kaddr = kmap_atomic(page); > memcpy(kaddr + offset, data, tocopy); >
On 10/16, Chao Yu wrote: > Hi Jaegeuk, > > On 2017/10/13 7:15, Jaegeuk Kim wrote: > > This patch returns an error number to quota_write in order for quota to handle > > it correctly. > > We should return error number like __generic_file_write_iter, right? it > needs to return written bytes if we have written one page or more, otherwise > return error number feedbacked from write_begin. > > So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on > f2fs_quota_write")? I thought like that, but realized the code change is somewhat different between them. Thanks, > > Thanks, > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > --- > > fs/f2fs/super.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 2feecf5e7f4c..840a0876005b 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, > > > > err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, > > &page, NULL); > > - if (unlikely(err)) > > + if (unlikely(err)) { > > + if (len == towrite) > > + return err; > > break; > > + } > > > > kaddr = kmap_atomic(page); > > memcpy(kaddr + offset, data, tocopy); > >
On 2017/10/17 7:04, Jaegeuk Kim wrote: > On 10/16, Chao Yu wrote: >> Hi Jaegeuk, >> >> On 2017/10/13 7:15, Jaegeuk Kim wrote: >>> This patch returns an error number to quota_write in order for quota to handle >>> it correctly. >> >> We should return error number like __generic_file_write_iter, right? it >> needs to return written bytes if we have written one page or more, otherwise >> return error number feedbacked from write_begin. >> >> So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on >> f2fs_quota_write")? > > I thought like that, but realized the code change is somewhat different between > them. Hmm... main structure of codes here is copied from other file systems, is there the same problem in *_quota_write of other file systems? BTW, it looks making below judgment condition being useless. if (len == towrite) return 0; Thanks, > > Thanks, > >> >> Thanks, >> >>> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/super.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 2feecf5e7f4c..840a0876005b 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, >>> >>> err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, >>> &page, NULL); >>> - if (unlikely(err)) >>> + if (unlikely(err)) { >>> + if (len == towrite) >>> + return err; >>> break; >>> + } >>> >>> kaddr = kmap_atomic(page); >>> memcpy(kaddr + offset, data, tocopy); >>> > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >
On 10/17, Chao Yu wrote: > > > On 2017/10/17 7:04, Jaegeuk Kim wrote: > > On 10/16, Chao Yu wrote: > >> Hi Jaegeuk, > >> > >> On 2017/10/13 7:15, Jaegeuk Kim wrote: > >>> This patch returns an error number to quota_write in order for quota to handle > >>> it correctly. > >> > >> We should return error number like __generic_file_write_iter, right? it > >> needs to return written bytes if we have written one page or more, otherwise > >> return error number feedbacked from write_begin. > >> > >> So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on > >> f2fs_quota_write")? > > > > I thought like that, but realized the code change is somewhat different between > > them. > > Hmm... main structure of codes here is copied from other file systems, is there > the same problem in *_quota_write of other file systems? > > BTW, it looks making below judgment condition being useless. > > if (len == towrite) > return 0; We need this to avoid needless inode updates. :P Thanks, > > Thanks, > > > > > Thanks, > > > >> > >> Thanks, > >> > >>> > >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > >>> --- > >>> fs/f2fs/super.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 2feecf5e7f4c..840a0876005b 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, > >>> > >>> err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, > >>> &page, NULL); > >>> - if (unlikely(err)) > >>> + if (unlikely(err)) { > >>> + if (len == towrite) > >>> + return err; > >>> break; > >>> + } > >>> > >>> kaddr = kmap_atomic(page); > >>> memcpy(kaddr + offset, data, tocopy); > >>> > > > > ------------------------------------------------------------------------------ > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > _______________________________________________ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >
On 2017/10/18 2:17, Jaegeuk Kim wrote: > On 10/17, Chao Yu wrote: >> >> >> On 2017/10/17 7:04, Jaegeuk Kim wrote: >>> On 10/16, Chao Yu wrote: >>>> Hi Jaegeuk, >>>> >>>> On 2017/10/13 7:15, Jaegeuk Kim wrote: >>>>> This patch returns an error number to quota_write in order for quota to handle >>>>> it correctly. >>>> >>>> We should return error number like __generic_file_write_iter, right? it >>>> needs to return written bytes if we have written one page or more, otherwise >>>> return error number feedbacked from write_begin. >>>> >>>> So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on >>>> f2fs_quota_write")? >>> >>> I thought like that, but realized the code change is somewhat different between >>> them. >> >> Hmm... main structure of codes here is copied from other file systems, is there >> the same problem in *_quota_write of other file systems? >> >> BTW, it looks making below judgment condition being useless. >> >> if (len == towrite) >> return 0; > > We need this to avoid needless inode updates. :P For err = 0 and len == towrite case, it more likes a bug of quota that passing 0 in @len. :(, Oh, still didn't get that why there is difference in between reverting and this fixing. Can you please explain more about this? Thanks, > > Thanks, > >> >> Thanks, >> >>> >>> Thanks, >>> >>>> >>>> Thanks, >>>> >>>>> >>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>>>> --- >>>>> fs/f2fs/super.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>> index 2feecf5e7f4c..840a0876005b 100644 >>>>> --- a/fs/f2fs/super.c >>>>> +++ b/fs/f2fs/super.c >>>>> @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, >>>>> >>>>> err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, >>>>> &page, NULL); >>>>> - if (unlikely(err)) >>>>> + if (unlikely(err)) { >>>>> + if (len == towrite) >>>>> + return err; >>>>> break; >>>>> + } >>>>> >>>>> kaddr = kmap_atomic(page); >>>>> memcpy(kaddr + offset, data, tocopy); >>>>> >>> >>> ------------------------------------------------------------------------------ >>> Check out the vibrant tech community on one of the world's most >>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot >>> _______________________________________________ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>> > > . >
On 10/18, Chao Yu wrote: > On 2017/10/18 2:17, Jaegeuk Kim wrote: > > On 10/17, Chao Yu wrote: > >> > >> > >> On 2017/10/17 7:04, Jaegeuk Kim wrote: > >>> On 10/16, Chao Yu wrote: > >>>> Hi Jaegeuk, > >>>> > >>>> On 2017/10/13 7:15, Jaegeuk Kim wrote: > >>>>> This patch returns an error number to quota_write in order for quota to handle > >>>>> it correctly. > >>>> > >>>> We should return error number like __generic_file_write_iter, right? it > >>>> needs to return written bytes if we have written one page or more, otherwise > >>>> return error number feedbacked from write_begin. > >>>> > >>>> So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on > >>>> f2fs_quota_write")? > >>> > >>> I thought like that, but realized the code change is somewhat different between > >>> them. > >> > >> Hmm... main structure of codes here is copied from other file systems, is there > >> the same problem in *_quota_write of other file systems? > >> > >> BTW, it looks making below judgment condition being useless. > >> > >> if (len == towrite) > >> return 0; > > > > We need this to avoid needless inode updates. :P > > For err = 0 and len == towrite case, it more likes a bug of quota that passing > 0 in @len. > > :(, Oh, still didn't get that why there is difference in between reverting and > this fixing. Can you please explain more about this? Ah, right. Let me just revert the original patch. :) Thanks,
On 2017/10/20 3:08, Jaegeuk Kim wrote: > On 10/18, Chao Yu wrote: >> On 2017/10/18 2:17, Jaegeuk Kim wrote: >>> On 10/17, Chao Yu wrote: >>>> >>>> >>>> On 2017/10/17 7:04, Jaegeuk Kim wrote: >>>>> On 10/16, Chao Yu wrote: >>>>>> Hi Jaegeuk, >>>>>> >>>>>> On 2017/10/13 7:15, Jaegeuk Kim wrote: >>>>>>> This patch returns an error number to quota_write in order for quota to handle >>>>>>> it correctly. >>>>>> >>>>>> We should return error number like __generic_file_write_iter, right? it >>>>>> needs to return written bytes if we have written one page or more, otherwise >>>>>> return error number feedbacked from write_begin. >>>>>> >>>>>> So how about reverting 4f31d26b0c17 ("f2fs: return wrong error number on >>>>>> f2fs_quota_write")? >>>>> >>>>> I thought like that, but realized the code change is somewhat different between >>>>> them. >>>> >>>> Hmm... main structure of codes here is copied from other file systems, is there >>>> the same problem in *_quota_write of other file systems? >>>> >>>> BTW, it looks making below judgment condition being useless. >>>> >>>> if (len == towrite) >>>> return 0; >>> >>> We need this to avoid needless inode updates. :P >> >> For err = 0 and len == towrite case, it more likes a bug of quota that passing >> 0 in @len. >> >> :(, Oh, still didn't get that why there is difference in between reverting and >> this fixing. Can you please explain more about this? > > Ah, right. Let me just revert the original patch. :) Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, > > Thanks, >
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 2feecf5e7f4c..840a0876005b 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1397,8 +1397,11 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type, err = a_ops->write_begin(NULL, mapping, off, tocopy, 0, &page, NULL); - if (unlikely(err)) + if (unlikely(err)) { + if (len == towrite) + return err; break; + } kaddr = kmap_atomic(page); memcpy(kaddr + offset, data, tocopy);
This patch returns an error number to quota_write in order for quota to handle it correctly. Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/super.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)