Hi! On Thu, Nov 28 2024, Alex Markuze wrote: > On Thu, Nov 28, 2024 at 7:43 PM Luis Henriques <luis.henriques@xxxxxxxxx> wrote: >> >> Hi Alex, >> >> [ Thank you for looking into this. ] >> >> On Wed, Nov 27 2024, Alex Markuze wrote: >> >> > Hi, Folks. >> > AFAIK there is no side effect that can affect MDS with this fix. >> > This crash happens following this patch >> > "1065da21e5df9d843d2c5165d5d576be000142a6" "ceph: stop copying to iter >> > at EOF on sync reads". >> > >> > Per your fix Luis, it seems to address only the cases when i_size goes >> > to zero but can happen anytime the `i_size` goes below `off`. >> > I propose fixing it this way: >> >> Hmm... you're probably right. I didn't see this happening, but I guess it >> could indeed happen. >> >> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> > index 4b8d59ebda00..19b084212fee 100644 >> > --- a/fs/ceph/file.c >> > +++ b/fs/ceph/file.c >> > @@ -1066,7 +1066,7 @@ ssize_t __ceph_sync_read(struct inode *inode, >> > loff_t *ki_pos, >> > if (ceph_inode_is_shutdown(inode)) >> > return -EIO; >> > >> > - if (!len) >> > + if (!len || !i_size) >> > return 0; >> > /* >> > * flush any page cache pages in this range. this >> > @@ -1200,12 +1200,11 @@ ssize_t __ceph_sync_read(struct inode *inode, >> > loff_t *ki_pos, >> > } >> > >> > idx = 0; >> > - if (ret <= 0) >> > - left = 0; >> >> Right now I don't have any means for testing this patch. However, I don't >> think this is completely correct. By removing the above condition you're >> discarding cases where an error has occurred (i.e. where ret is negative). > > I didn't discard it though :). > I folded it into the `if` statement. I find the if else construct > overly verbose and cumbersome. > > + left = (ret > 0) ? ret : 0; > Right, but with your patch, if 'ret < 0', we could still hit the first branch instead of that one: if (off + ret > i_size) left = (i_size > off) ? i_size - off : 0; else left = (ret > 0) ? ret : 0; Cheers, -- Luís
